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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 1 Jan 2021 10:21:24 -0600
> Cc: 45562@debbugs.gnu.org
> 
> >> -  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.

A user who builds his/her own Emacs could modify the source to add
some buffer-local variable and overflow MAX_PER_BUFFER_VARS.  If they
build with optimizations, we still want to abort, right?

> >> --- 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.

Or maybe the test is in error, and it should say

  if (v1 == 0)