bug#45562: [PATCH] Fix "comparison always the same" warnings found by lgtm

[Stefan Kangas <stefan@marxist.se>]

Eli Zaretskii <eliz@gnu.org> writes:

>> The attached patch fixes some warnings found by lgtm.com.
>
> Thanks.  IME, these tools have quite a low signal-to-noise ratio.  In
> this case:

Thanks for the review!  Indeed, this is why I asked for some comments.

>> --- a/src/buffer.c
>> +++ b/src/buffer.c
>> @@ -5238,8 +5238,7 @@ init_buffer_once (void)
>>    PDUMPER_REMEMBER_SCALAR (buffer_local_flags);
>>
>>    /* Need more room? */
>> -  if (idx >= MAX_PER_BUFFER_VARS)
>> -    emacs_abort ();
>> +  eassert (idx < MAX_PER_BUFFER_VARS);
>
> This is wrong, because eassert compiles to nothing in the production
> build, so it is only good for situations where continuing without
> aborting will do something reasonable, or if it will crash anyway in
> the very next source line.  In this case, there's no way we can
> continue, and the programmer evidently wanted us to abort rather than
> continue and let us crash later.

Right.  But we know the value of both idx and MAX_PER_BUFFER_VARS at
compile time.  So while I understand your argument, it is arguably a
judgment call whether or not it is worth making this check also in
production builds.  IMHO, the eassert has the (minor) benefit of making
the intention clearer.

That said, AFAICT we call this function only once per lifetime.  So I'm
happy to leave this out if you prefer.

>> --- a/src/fns.c
>> +++ b/src/fns.c
>> @@ -3847,8 +3847,6 @@ base64_decode_1 (const char *from, char *to, ptrdiff_t length,
>>        if (c == '=')
>>  	continue;
>>
>> -      if (v1 < 0)
>> -	return -1;
>>        value += v1 - 1;
>>
>>        c = value & 0xff;
>
> I don't think I see why removing the test and the failure return would
> be TRT.  What did I miss?

Because we have above:

do { ... } while (v1 < 0);

So unless I am missing something the test is always false and we will
never reach the return.