GNU bug report logs - #45562
[PATCH] Fix "comparison always the same" warnings found by lgtm

Previous Next

Package: emacs;

Reported by: Stefan Kangas <stefan <at> marxist.se>

Date: Thu, 31 Dec 2020 08:34:01 UTC

Severity: wishlist

Tags: patch

Fixed in version 28.1

Done: Lars Ingebrigtsen <larsi <at> gnus.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 45562 in the body.
You can then email your comments to 45562 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Thu, 31 Dec 2020 08:34:01 GMT) Full text and rfc822 format available.

Acknowledgement sent to Stefan Kangas <stefan <at> marxist.se>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Thu, 31 Dec 2020 08:34:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix "comparison always the same" warnings found by lgtm
Date: Thu, 31 Dec 2020 00:33:06 -0800
[Message part 1 (text/plain, inline)]
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/
[0001-Fix-comparison-always-the-same-warnings-found-by-lgt.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Thu, 31 Dec 2020 14:14:02 GMT) Full text and rfc822 format available.

Message #8 received at 45562 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 45562 <at> debbugs.gnu.org
Subject: Re: bug#45562: [PATCH] Fix "comparison always the same" warnings
 found by lgtm
Date: Thu, 31 Dec 2020 16:12:51 +0200
> From: Stefan Kangas <stefan <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Fri, 01 Jan 2021 11:09:01 GMT) Full text and rfc822 format available.

Message #11 received at 45562 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 45562 <at> debbugs.gnu.org, Stefan Kangas <stefan <at> marxist.se>
Subject: Re: bug#45562: [PATCH] Fix "comparison always the same" warnings
 found by lgtm
Date: Fri, 01 Jan 2021 12:08:46 +0100
Eli Zaretskii <eliz <at> 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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Fri, 01 Jan 2021 11:38:02 GMT) Full text and rfc822 format available.

Message #14 received at 45562 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 45562 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Kangas <stefan <at> marxist.se>
Subject: Re: bug#45562: [PATCH] Fix "comparison always the same" warnings
 found by lgtm
Date: Fri, 01 Jan 2021 12:37:43 +0100
Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Fri, 01 Jan 2021 12:06:02 GMT) Full text and rfc822 format available.

Message #17 received at 45562 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 45562 <at> debbugs.gnu.org, stefan <at> marxist.se
Subject: Re: bug#45562: [PATCH] Fix "comparison always the same" warnings
 found by lgtm
Date: Fri, 01 Jan 2021 14:05:09 +0200
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: Stefan Kangas <stefan <at> marxist.se>,  45562 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Fri, 01 Jan 2021 16:11:02 GMT) Full text and rfc822 format available.

Message #20 received at 45562 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Daniel Martín <mardani29 <at> yahoo.es>, 
 Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 45562 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#45562: [PATCH] Fix "comparison always the same" warnings
 found by lgtm
Date: Fri, 1 Jan 2021 10:10:38 -0600
Daniel Martín <mardani29 <at> yahoo.es> writes:

> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
>> Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Fri, 01 Jan 2021 16:22:01 GMT) Full text and rfc822 format available.

Message #23 received at 45562 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 45562 <at> debbugs.gnu.org
Subject: Re: bug#45562: [PATCH] Fix "comparison always the same" warnings
 found by lgtm
Date: Fri, 1 Jan 2021 10:21:24 -0600
Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Fri, 01 Jan 2021 16:39:02 GMT) Full text and rfc822 format available.

Message #26 received at 45562 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 45562 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#45562: [PATCH] Fix "comparison always the same" warnings
 found by lgtm
Date: Fri, 01 Jan 2021 17:38:28 +0100
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 <at> linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Fri, 01 Jan 2021 18:18:02 GMT) Full text and rfc822 format available.

Message #29 received at 45562 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 45562 <at> debbugs.gnu.org
Subject: Re: bug#45562: [PATCH] Fix "comparison always the same" warnings
 found by lgtm
Date: Fri, 01 Jan 2021 20:17:35 +0200
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Fri, 1 Jan 2021 10:21:24 -0600
> Cc: 45562 <at> 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)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45562; Package emacs. (Wed, 21 Jul 2021 11:31:02 GMT) Full text and rfc822 format available.

Message #32 received at 45562 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 45562 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Kangas <stefan <at> marxist.se>
Subject: Re: bug#45562: [PATCH] Fix "comparison always the same" warnings
 found by lgtm
Date: Wed, 21 Jul 2021 13:30:10 +0200
Andreas Schwab <schwab <at> 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




bug marked as fixed in version 28.1, send any further explanations to 45562 <at> debbugs.gnu.org and Stefan Kangas <stefan <at> marxist.se> Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Wed, 21 Jul 2021 11:31:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Thu, 19 Aug 2021 11:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 223 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.