Re: [PATCH] Fix many compiler warnings

[Markus Wichmann <nullplan@gmx.net>]

On Sat, Nov 10, 2018 at 05:48:23PM +0100, Sedrubal wrote:
> Hello,
> 
> I'm building musl for aarch64 with clang and get many compiler warnings
> (mostly Wbitwise-op-parentheses, Wshift-op-parentheses and Wlogical-op-parentheses).
> I tried to fix them (and I hope, I did not introduce new bugs).
> 

Well, hope belongs in the church. Whether or not you introduced bugs is
a thing you can test for. There is libc-test.

I don't know Rich's position on warnings, but my understanding was he
only cares about some of them. Writing code warning-free on multiple
platforms with multiple compilers is damn near impossible, anyway. (E.g.
take the following snippet:

switch (x)
{
    case foo:
        return other_func();
        break;
}

At work I have a compiler that will complain about the unreachable
break. If I deleted it I have another compiler (and a company directive)
warning that I have a case-statement that is not terminated with a
break.)

> 
> diff --git a/include/byteswap.h b/include/byteswap.h
> index 00b9df3c..f2dbb912 100644
> --- a/include/byteswap.h
> +++ b/include/byteswap.h
> @@ -11,12 +11,12 @@ static __inline uint16_t __bswap_16(uint16_t __x)
>  
>  static __inline uint32_t __bswap_32(uint32_t __x)
>  {
> -	return __x>>24 | __x>>8&0xff00 | __x<<8&0xff0000 | __x<<24;
> +	return __x>>24 | __x>>(8&0xff00) | __x<<(8&0xff0000) | __x<<24;
>  }
>  

That's erroneous and you can actually see that pretty easily: The part
in the parentheses must always be zero. Could that possibly have been
the intention? No. (And this change fixed a warning? Shows what those
are worth!)

I guess the following will do the trick:

return (__x>>24) | (__x>>8 & 0xff00) | (__x<<8 & 0xff0000) | (__x<<24);

>  static __inline uint64_t __bswap_64(uint64_t __x)
>  {
> -	return __bswap_32(__x)+0ULL<<32 | __bswap_32(__x>>32);
> +	return (__bswap_32(__x)+0ULL)<<32 | __bswap_32(__x>>32);
>  }
>  
>  #define bswap_16(x) __bswap_16(x)
> diff --git a/include/endian.h b/include/endian.h
> index 1bd44451..6957fad3 100644
> --- a/include/endian.h
> +++ b/include/endian.h
> @@ -24,17 +24,17 @@
>  
>  static __inline uint16_t __bswap16(uint16_t __x)
>  {
> -	return __x<<8 | __x>>8;
> +	return (__x << 8) | (__x >> 8);
>  }
>  
>  static __inline uint32_t __bswap32(uint32_t __x)
>  {
> -	return __x>>24 | __x>>8&0xff00 | __x<<8&0xff0000 | __x<<24;
> +	return (__x>>24) | ((__x>>8)&0xff00) | ((__x<<8)&0xff0000) | (__x<<24);
>  }
>  

You did it correctly here. The why the error above?

> diff --git a/src/locale/__mo_lookup.c b/src/locale/__mo_lookup.c
> index d18ab774..8750fd2c 100644
> --- a/src/locale/__mo_lookup.c
> +++ b/src/locale/__mo_lookup.c
> @@ -3,7 +3,7 @@
>  
>  static inline uint32_t swapc(uint32_t x, int c)
>  {
> -	return c ? x>>24 | x>>8&0xff00 | x<<8&0xff0000 | x<<24 : x;
> +	return c ? x>>24 | x>>(8&0xff00) | x<<(8&0xff0000) | x<<24 : x;
>  }
>  

Again, as above.

> diff --git a/src/locale/dcngettext.c b/src/locale/dcngettext.c
> index 8b891d00..39c0b191 100644
> --- a/src/locale/dcngettext.c
> +++ b/src/locale/dcngettext.c
> @@ -176,7 +176,7 @@ notrans:
>  			snprintf(name, sizeof name, "%s/%.*s%.*s/%s/%s.mo\0",
>  				dirname, (int)loclen, locname,
>  				(int)alt_modlen, modname, catname, domainname);
> -			if (map = __map_file(name, &map_size)) break;
> +			if ((map = __map_file(name, &map_size))) break;
>  

Actually, why is there an if-condition assignment here? I've done that
in the past, but only in complex conditions, not simple ones like this.
Wouldn't

    map = __map_file(name, &map_size);
    if (map) break;

be better style here? I'm not against assigning in conditions in
general, but it should serve a purpose. "while ((de = readdir(d)))" has
such a purpose, but that is a while-condition.

> diff --git a/src/locale/iconv.c b/src/locale/iconv.c
> index 3047c27b..d189f598 100644
> --- a/src/locale/iconv.c
> +++ b/src/locale/iconv.c
> @@ -181,7 +181,7 @@ static void put_16(unsigned char *s, unsigned c, int e)
>  static unsigned get_32(const unsigned char *s, int e)
>  {
>  	e &= 3;
> -	return s[e]+0U<<24 | s[e^1]<<16 | s[e^2]<<8 | s[e^3];
> +	return (s[e]+0U)<<24 | s[e^1]<<16 | s[e^2]<<8 | s[e^3];
>  }
>  

Wait, I'm confused again. Why is it necessary to convert the first
dereference of s to unsigned int, but not the others?

>  static void put_32(unsigned char *s, unsigned c, int e)
> @@ -201,7 +201,7 @@ static unsigned legacy_map(const unsigned char *map, unsigned c)
>  {
>  	if (c < 4*map[-1]) return c;
>  	unsigned x = c - 4*map[-1];
> -	x = map[x*5/4]>>2*x%8 | map[x*5/4+1]<<8-2*x%8 & 1023;
> +	x = (map[x*5/4]>>(2*x%8)) | ((map[x*5/4+1]<<(8-2*x%8)) & 1023);
>  	return x < 256 ? x : legacy_chars[x-256];
>  }
>  

I've stared at the old line for five minutes now, and I still don't know
what it does. As such I can't evaluate the consistency of the new
version. What were you thinking when you wrote that line, Rich?

Let's see here... bitshift binds less strongly than arithmetic, and
bitlogic binds even less strongly than that. So mechanically, the new
line is correct. I still don't know what it does.

Ciao,
Markus