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

Previous Next

Full log

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.

Previous Next

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