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

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

Severity: wishlist

The attached patch fixes some warnings found by lgtm.com.

Does it look okay?

Ref: https://lgtm.com/projects/g/emacs-mirror/emacs/

[-- Attachment #2: 0001-Fix-comparison-always-the-same-warnings-found-by-lgt.patch --]
[-- Type: text/x-diff, Size: 2685 bytes --]

From 340206865383b0764a7d07cc7771f4fb8773b302 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Thu, 31 Dec 2020 08:19:41 +0100
Subject: [PATCH] Fix "comparison always the same" warnings found by lgtm

* src/alloc.c (memory_full):
* src/buffer.c (init_buffer_once):
* src/fns.c (base64_decode_1):
* src/window.c (window_scroll_pixel_based):
* src/xfaces.c (merge_face_vectors): Fix warning "Comparison result is
always the same" found by lgtm dot com.
---
 src/alloc.c  | 2 +-
 src/buffer.c | 3 +--
 src/fns.c    | 2 --
 src/window.c | 2 +-
 src/xfaces.c | 2 +-
 5 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 0b387dd8c1..5386a142de 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4001,7 +4001,7 @@ memory_full (size_t nbytes)
 	  {
 	    if (i == 0)
 	      free (spare_memory[i]);
-	    else if (i >= 1 && i <= 4)
+	    else if (i <= 4)
 	      lisp_align_free (spare_memory[i]);
 	    else
 	      lisp_free (spare_memory[i]);
diff --git a/src/buffer.c b/src/buffer.c
index 9e44345616..8bca4977ca 100644
--- 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);
   last_per_buffer_idx = idx;
   PDUMPER_REMEMBER_SCALAR (last_per_buffer_idx);
 
diff --git a/src/fns.c b/src/fns.c
index 2de1d26dd3..f0cc164bc7 100644
--- 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;
diff --git a/src/window.c b/src/window.c
index 5db166e345..bfdbd426a6 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5708,7 +5708,7 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, bool noerror)
 		 && start_pos > BEGV)
 	    move_it_by_lines (&it, -1);
 	}
-      else if (dy > 0)
+      else /* if (dy > 0) */
 	{
 	  goal_y = it.current_y + dy;
 	  move_it_to (&it, ZV, -1, goal_y, -1, MOVE_TO_POS | MOVE_TO_Y);
diff --git a/src/xfaces.c b/src/xfaces.c
index 73a536b19c..ea0e9d6a28 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2228,7 +2228,7 @@ merge_face_vectors (struct window *w,
 	else if (i != LFACE_FONT_INDEX && ! EQ (to[i], from[i]))
 	  {
 	    to[i] = from[i];
-	    if (i >= LFACE_FAMILY_INDEX && i <= LFACE_SLANT_INDEX)
+	    if (i <= LFACE_SLANT_INDEX)
 	      font_clear_prop (to,
 	                       (i == LFACE_FAMILY_INDEX ? FONT_FAMILY_INDEX
 			        : i == LFACE_FOUNDRY_INDEX ? FONT_FOUNDRY_INDEX
-- 
2.29.2

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 31 Dec 2020 00:33:06 -0800
> 
> 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:

> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -4001,7 +4001,7 @@ memory_full (size_t nbytes)
>  	  {
>  	    if (i == 0)
>  	      free (spare_memory[i]);
> -	    else if (i >= 1 && i <= 4)
> +	    else if (i <= 4)
>  	      lisp_align_free (spare_memory[i]);
>  	    else
>  	      lisp_free (spare_memory[i]);

This is an optimization better left to the compiler, IMO.

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

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

> --- a/src/window.c
> +++ b/src/window.c
> @@ -5708,7 +5708,7 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, bool noerror)
>  		 && start_pos > BEGV)
>  	    move_it_by_lines (&it, -1);
>  	}
> -      else if (dy > 0)
> +      else /* if (dy > 0) */

I don't necessarily object, but this is again an optimization that
compilers are better at than people.

> --- a/src/xfaces.c
> +++ b/src/xfaces.c
> @@ -2228,7 +2228,7 @@ merge_face_vectors (struct window *w,
>  	else if (i != LFACE_FONT_INDEX && ! EQ (to[i], from[i]))
>  	  {
>  	    to[i] = from[i];
> -	    if (i >= LFACE_FAMILY_INDEX && i <= LFACE_SLANT_INDEX)
> +	    if (i <= LFACE_SLANT_INDEX)

This change hard-codes the assumption about the numerical value of
LFACE_FAMILY_INDEX, so it'd be a time bomb waiting to blow up.  For
example, imagine that the enumeration is modified such that the value
of LFACE_FAMILY_INDEX changes, or we are using a compiler with a
different scheme of assigning numerical values to enumeration
constants.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> --- a/src/alloc.c
>> +++ b/src/alloc.c
>> @@ -4001,7 +4001,7 @@ memory_full (size_t nbytes)
>>  	  {
>>  	    if (i == 0)
>>  	      free (spare_memory[i]);
>> -	    else if (i >= 1 && i <= 4)
>> +	    else if (i <= 4)
>>  	      lisp_align_free (spare_memory[i]);
>>  	    else
>>  	      lisp_free (spare_memory[i]);
>
> This is an optimization better left to the compiler, IMO.

I think the change made the code slightly clearer, though?  You don't
have to think about whether there's anything in the range between 0 and
>= 1.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Unknown]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> --- a/src/alloc.c
>>> +++ b/src/alloc.c
>>> @@ -4001,7 +4001,7 @@ memory_full (size_t nbytes)
>>>  	  {
>>>  	    if (i == 0)
>>>  	      free (spare_memory[i]);
>>> -	    else if (i >= 1 && i <= 4)
>>> +	    else if (i <= 4)
>>>  	      lisp_align_free (spare_memory[i]);
>>>  	    else
>>>  	      lisp_free (spare_memory[i]);
>>
>> This is an optimization better left to the compiler, IMO.
>
> I think the change made the code slightly clearer, though?  You don't
> have to think about whether there's anything in the range between 0 and
>>= 1.

I think it depends on the programmer.  To me, the original code makes
more clear that the branch runs when i is in the [1..4] range, in a
mathematical sense.

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Kangas <stefan@marxist.se>,  45562@debbugs.gnu.org
> Date: Fri, 01 Jan 2021 12:08:46 +0100
> 
> >> --- a/src/alloc.c
> >> +++ b/src/alloc.c
> >> @@ -4001,7 +4001,7 @@ memory_full (size_t nbytes)
> >>  	  {
> >>  	    if (i == 0)
> >>  	      free (spare_memory[i]);
> >> -	    else if (i >= 1 && i <= 4)
> >> +	    else if (i <= 4)
> >>  	      lisp_align_free (spare_memory[i]);
> >>  	    else
> >>  	      lisp_free (spare_memory[i]);
> >
> > This is an optimization better left to the compiler, IMO.
> 
> I think the change made the code slightly clearer, though?  You don't
> have to think about whether there's anything in the range between 0 and
> >= 1.

If you like the modified code better, I won't object.

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

[Stefan Kangas]

Daniel Martín <mardani29@yahoo.es> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> --- a/src/alloc.c
>>>> +++ b/src/alloc.c
>>>> @@ -4001,7 +4001,7 @@ memory_full (size_t nbytes)
>>>>  	  {
>>>>  	    if (i == 0)
>>>>  	      free (spare_memory[i]);
>>>> -	    else if (i >= 1 && i <= 4)
>>>> +	    else if (i <= 4)
>>>>  	      lisp_align_free (spare_memory[i]);
>>>>  	    else
>>>>  	      lisp_free (spare_memory[i]);
>>>
>>> This is an optimization better left to the compiler, IMO.
>>
>> I think the change made the code slightly clearer, though?  You don't
>> have to think about whether there's anything in the range between 0 and
>>>= 1.
>
> I think it depends on the programmer.  To me, the original code makes
> more clear that the branch runs when i is in the [1..4] range, in a
> mathematical sense.

I liked the new one better myself, but we should probably just leave it
alone if we can't agree.  The difference is minor, in any case.

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

[Stefan Kangas]

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.

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

[Andreas Schwab]

On Jan 01 2021, Stefan Kangas wrote:

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

Looking at commit 5abaea334cf, that likely needs to test v1 == 0 instead.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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)

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

[Lars Ingebrigtsen]

Andreas Schwab <schwab@linux-m68k.org> writes:

>>>> --- 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;
>
> Looking at commit 5abaea334cf, that likely needs to test v1 == 0 instead.

Seems like so to me, too.  So I've now made that change on the trunk --
so the lgtm checks found a real bug there.

As for the others, skimming this thread there didn't seem to be any
consensus that the proposed changes makes the code any better (or
clearer), so applying those doesn't seem to be a net win, and I'm
closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no