[PATCH] Fix many compiler warnings

[PATCH] Fix many compiler warnings

[Sedrubal]

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

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

My configure command is following (I don't know if it matters):
```
CROSS_COMPILE=llvm- \
CC=clang \
CFLAGS="-target arm64v8a-arm-none-eabi -std=c99" \
./configure \
    --disable-shared \
    --disable-optimize \
    --enable-debug \
    --target=aarch64
```

Signed-off-by: sedrubal <dev@sedrubal.de>
---
 include/byteswap.h          |  4 ++--
 include/endian.h            |  6 +++---
 src/ctype/towctrans.c       |  6 ++++--
 src/internal/shgetc.c       |  2 +-
 src/legacy/getpass.c        |  2 +-
 src/locale/__mo_lookup.c    |  2 +-
 src/locale/dcngettext.c     |  2 +-
 src/locale/iconv.c          |  6 +++---
 src/locale/locale_map.c     |  6 +++---
 src/multibyte/c16rtomb.c    |  2 +-
 src/network/dn_skipname.c   |  6 ++++--
 src/passwd/getgr_a.c        |  8 ++++----
 src/passwd/getpw_a.c        |  8 ++++----
 src/prng/__rand48_step.c    |  4 ++--
 src/regex/glob.c            |  2 +-
 src/select/select.c         |  2 +-
 src/stdio/getc.h            |  2 +-
 src/stdio/putc.h            |  2 +-
 src/stdio/vfprintf.c        | 16 ++++++++--------
 src/stdio/vfwprintf.c       | 16 ++++++++--------
 src/thread/pthread_cancel.c |  2 +-
 21 files changed, 55 insertions(+), 51 deletions(-)

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;
 }
 
 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);
 }
 
 static __inline uint64_t __bswap64(uint64_t __x)
 {
-	return __bswap32(__x)+0ULL<<32 | __bswap32(__x>>32);
+	return ((__bswap32(__x) + 0ULL) << 32) | __bswap32(__x >> 32);
 }
 
 #if __BYTE_ORDER == __LITTLE_ENDIAN
diff --git a/src/ctype/towctrans.c b/src/ctype/towctrans.c
index 8f681018..bd0136dd 100644
--- a/src/ctype/towctrans.c
+++ b/src/ctype/towctrans.c
@@ -259,12 +259,14 @@ static wchar_t __towcase(wchar_t wc, int lower)
 	 || (unsigned)wc - 0xabc0 <= 0xfeff-0xabc0)
 		return wc;
 	/* special case because the diff between upper/lower is too big */
-	if (lower && (unsigned)wc - 0x10a0 < 0x2e)
+	if (lower && (unsigned)wc - 0x10a0 < 0x2e) {
 		if (wc>0x10c5 && wc != 0x10c7 && wc != 0x10cd) return wc;
 		else return wc + 0x2d00 - 0x10a0;
-	if (!lower && (unsigned)wc - 0x2d00 < 0x26)
+	}
+	if (!lower && (unsigned)wc - 0x2d00 < 0x26) {
 		if (wc>0x2d25 && wc != 0x2d27 && wc != 0x2d2d) return wc;
 		else return wc + 0x10a0 - 0x2d00;
+	}
 	if (lower && (unsigned)wc - 0x13a0 < 0x50)
 		return wc + 0xab70 - 0x13a0;
 	if (!lower && (unsigned)wc - 0xab70 < 0x50)
diff --git a/src/internal/shgetc.c b/src/internal/shgetc.c
index ebd5fae7..c6aefed9 100644
--- a/src/internal/shgetc.c
+++ b/src/internal/shgetc.c
@@ -20,7 +20,7 @@ int __shgetc(FILE *f)
 {
 	int c;
 	off_t cnt = shcnt(f);
-	if (f->shlim && cnt >= f->shlim || (c=__uflow(f)) < 0) {
+	if ((f->shlim && cnt >= f->shlim) || (c=__uflow(f)) < 0) {
 		f->shcnt = f->buf - f->rpos + cnt;
 		f->shend = 0;
 		return EOF;
diff --git a/src/legacy/getpass.c b/src/legacy/getpass.c
index d51286c0..036a7859 100644
--- a/src/legacy/getpass.c
+++ b/src/legacy/getpass.c
@@ -27,7 +27,7 @@ char *getpass(const char *prompt)
 
 	l = read(fd, password, sizeof password);
 	if (l >= 0) {
-		if (l > 0 && password[l-1] == '\n' || l==sizeof password) l--;
+		if ((l > 0 && password[l-1] == '\n') || l==sizeof password) l--;
 		password[l] = 0;
 	}
 
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;
 }
 
 const char *__mo_lookup(const void *p, size_t size, const char *s)
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;
 
 			/* Try dropping @mod, _YY, then both. */
 			if (alt_modlen) {
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];
 }
 
 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];
 }
 
@@ -495,7 +495,7 @@ size_t iconv(iconv_t cd, char **restrict in, size_t *restrict inb, char **restri
 			if (c >= 93 || d >= 94) {
 				c += (0xa1-0x81);
 				d += 0xa1;
-				if (c >= 93 || c>=0xc6-0x81 && d>0x52)
+				if (c >= 93 || ((c >= 0xc6 - 0x81) && d>0x52))
 					goto ilseq;
 				if (d-'A'<26) d = d-'A';
 				else if (d-'a'<26) d = d-'a'+26;
diff --git a/src/locale/locale_map.c b/src/locale/locale_map.c
index 2321bac0..8ee4641c 100644
--- a/src/locale/locale_map.c
+++ b/src/locale/locale_map.c
@@ -32,9 +32,9 @@ const struct __locale_map *__get_locale(int cat, const char *val)
 	size_t l, n;
 
 	if (!*val) {
-		(val = getenv("LC_ALL")) && *val ||
-		(val = getenv(envvars[cat])) && *val ||
-		(val = getenv("LANG")) && *val ||
+		((val = getenv("LC_ALL")) && *val) ||
+		((val = getenv(envvars[cat])) && *val) ||
+		((val = getenv("LANG")) && *val) ||
 		(val = "C.UTF-8");
 	}
 
diff --git a/src/multibyte/c16rtomb.c b/src/multibyte/c16rtomb.c
index 39ca3758..5ebfbc29 100644
--- a/src/multibyte/c16rtomb.c
+++ b/src/multibyte/c16rtomb.c
@@ -15,7 +15,7 @@ size_t c16rtomb(char *restrict s, char16_t c16, mbstate_t *restrict ps)
 	}
 
 	if (!*x && c16 - 0xd800u < 0x400) {
-		*x = c16 - 0xd7c0 << 10;
+		*x = (c16 - 0xd7c0) << 10;
 		return 0;
 	}
 
diff --git a/src/network/dn_skipname.c b/src/network/dn_skipname.c
index d54c2e5d..44f8e68c 100644
--- a/src/network/dn_skipname.c
+++ b/src/network/dn_skipname.c
@@ -3,10 +3,12 @@
 int dn_skipname(const unsigned char *s, const unsigned char *end)
 {
 	const unsigned char *p;
-	for (p=s; p<end; p++)
+	for (p=s; p<end; p++) {
 		if (!*p) return p-s+1;
-		else if (*p>=192)
+		else if (*p>=192) {
 			if (p+1<end) return p-s+2;
 			else break;
+		}
+	}
 	return -1;
 }
diff --git a/src/passwd/getgr_a.c b/src/passwd/getgr_a.c
index afeb1ece..5605b4b6 100644
--- a/src/passwd/getgr_a.c
+++ b/src/passwd/getgr_a.c
@@ -33,8 +33,8 @@ int __getgr_a(const char *name, gid_t gid, struct group *gr, char **buf, size_t
 	}
 
 	while (!(rv = __getgrent_a(f, gr, buf, size, mem, nmem, res)) && *res) {
-		if (name && !strcmp(name, (*res)->gr_name)
-		|| !name && (*res)->gr_gid == gid) {
+		if ((name && !strcmp(name, (*res)->gr_name))
+		|| (!name && (*res)->gr_gid == gid)) {
 			break;
 		}
 	}
@@ -149,8 +149,8 @@ int __getgr_a(const char *name, gid_t gid, struct group *gr, char **buf, size_t
 			goto cleanup_f;
 		}
 
-		if (name && strcmp(name, gr->gr_name)
-		|| !name && gid != gr->gr_gid) {
+		if ((name && strcmp(name, gr->gr_name))
+		|| (!name && gid != gr->gr_gid)) {
 			rv = EIO;
 			goto cleanup_f;
 		}
diff --git a/src/passwd/getpw_a.c b/src/passwd/getpw_a.c
index 15a70c03..e1406319 100644
--- a/src/passwd/getpw_a.c
+++ b/src/passwd/getpw_a.c
@@ -34,8 +34,8 @@ int __getpw_a(const char *name, uid_t uid, struct passwd *pw, char **buf, size_t
 	}
 
 	while (!(rv = __getpwent_a(f, pw, buf, size, res)) && *res) {
-		if (name && !strcmp(name, (*res)->pw_name)
-		|| !name && (*res)->pw_uid == uid)
+		if ((name && !strcmp(name, (*res)->pw_name))
+		|| (!name && (*res)->pw_uid == uid))
 			break;
 	}
 	fclose(f);
@@ -122,8 +122,8 @@ int __getpw_a(const char *name, uid_t uid, struct passwd *pw, char **buf, size_t
 			goto cleanup_f;
 		}
 
-		if (name && strcmp(name, pw->pw_name)
-		|| !name && uid != pw->pw_uid) {
+		if ((name && strcmp(name, pw->pw_name))
+		|| (!name && uid != pw->pw_uid)) {
 			rv = EIO;
 			goto cleanup_f;
 		}
diff --git a/src/prng/__rand48_step.c b/src/prng/__rand48_step.c
index 94703d07..745fb0ee 100644
--- a/src/prng/__rand48_step.c
+++ b/src/prng/__rand48_step.c
@@ -4,8 +4,8 @@
 uint64_t __rand48_step(unsigned short *xi, unsigned short *lc)
 {
 	uint64_t a, x;
-	x = xi[0] | xi[1]+0U<<16 | xi[2]+0ULL<<32;
-	a = lc[0] | lc[1]+0U<<16 | lc[2]+0ULL<<32;
+	x = xi[0] | ((xi[1] + 0U) << 16) | ((xi[2] + 0ULL) << 32);
+	a = lc[0] | ((lc[1] + 0U) << 16) | ((lc[2] + 0ULL) << 32);
 	x = a*x + lc[3];
 	xi[0] = x;
 	xi[1] = x>>16;
diff --git a/src/regex/glob.c b/src/regex/glob.c
index aa1c6a44..72c9b5dc 100644
--- a/src/regex/glob.c
+++ b/src/regex/glob.c
@@ -142,7 +142,7 @@ static int do_glob(char *buf, size_t pos, int type, char *pat, int flags, int (*
 		/* With GLOB_PERIOD, don't allow matching . or .. unless
 		 * fnmatch would match them with FNM_PERIOD rules in effect. */
 		if (p2 && (flags & GLOB_PERIOD) && de->d_name[0]=='.'
-		    && (!de->d_name[1] || de->d_name[1]=='.' && !de->d_name[2])
+		    && (!de->d_name[1] || (de->d_name[1]=='.' && !de->d_name[2]))
 		    && fnmatch(pat, de->d_name, fnm_flags | FNM_PERIOD))
 			continue;
 
diff --git a/src/select/select.c b/src/select/select.c
index 02fd75c3..c913fe6b 100644
--- a/src/select/select.c
+++ b/src/select/select.c
@@ -16,7 +16,7 @@ int select(int n, fd_set *restrict rfds, fd_set *restrict wfds, fd_set *restrict
 			return __syscall_ret(-EINVAL);
 		time_t extra_secs = tv->tv_usec / 1000000;
 		ts.tv_nsec = tv->tv_usec % 1000000 * 1000;
-		const time_t max_time = (1ULL<<8*sizeof(time_t)-1)-1;
+		const time_t max_time = (1ULL << (8 * sizeof(time_t) - 1)) - 1;
 		ts.tv_sec = extra_secs > max_time - tv->tv_sec ?
 			max_time : tv->tv_sec + extra_secs;
 	}
diff --git a/src/stdio/getc.h b/src/stdio/getc.h
index e24f9905..7a4643cd 100644
--- a/src/stdio/getc.h
+++ b/src/stdio/getc.h
@@ -16,7 +16,7 @@ static int locking_getc(FILE *f)
 static inline int do_getc(FILE *f)
 {
 	int l = f->lock;
-	if (l < 0 || l && (l & ~MAYBE_WAITERS) == __pthread_self()->tid)
+	if (l < 0 || (l && (l & ~MAYBE_WAITERS) == __pthread_self()->tid))
 		return getc_unlocked(f);
 	return locking_getc(f);
 }
diff --git a/src/stdio/putc.h b/src/stdio/putc.h
index 2014c4ec..2d7dce16 100644
--- a/src/stdio/putc.h
+++ b/src/stdio/putc.h
@@ -16,7 +16,7 @@ static int locking_putc(int c, FILE *f)
 static inline int do_putc(int c, FILE *f)
 {
 	int l = f->lock;
-	if (l < 0 || l && (l & ~MAYBE_WAITERS) == __pthread_self()->tid)
+	if (l < 0 || (l && (l & ~MAYBE_WAITERS) == __pthread_self()->tid))
 		return putc_unlocked(c, f);
 	return locking_putc(c, f);
 }
diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c
index 9b961e7f..695f8ed5 100644
--- a/src/stdio/vfprintf.c
+++ b/src/stdio/vfprintf.c
@@ -19,12 +19,12 @@
 /* Convenient bit representation for modifier flags, which all fall
  * within 31 codepoints of the space character. */
 
-#define ALT_FORM   (1U<<'#'-' ')
-#define ZERO_PAD   (1U<<'0'-' ')
-#define LEFT_ADJ   (1U<<'-'-' ')
-#define PAD_POS    (1U<<' '-' ')
-#define MARK_POS   (1U<<'+'-' ')
-#define GROUPED    (1U<<'\''-' ')
+#define ALT_FORM   (1U << ('#' - ' '))
+#define ZERO_PAD   (1U << ('0' - ' '))
+#define LEFT_ADJ   (1U << ('-' - ' '))
+#define PAD_POS    (1U << (' ' - ' '))
+#define MARK_POS   (1U << ('+' - ' '))
+#define GROUPED    (1U << ('\'' - ' '))
 
 #define FLAGMASK (ALT_FORM|ZERO_PAD|LEFT_ADJ|PAD_POS|MARK_POS|GROUPED)
 
@@ -471,8 +471,8 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 		}
 
 		/* Read modifier flags */
-		for (fl=0; (unsigned)*s-' '<32 && (FLAGMASK&(1U<<*s-' ')); s++)
-			fl |= 1U<<*s-' ';
+		for (fl=0; ((unsigned)*s-' ')<32 && (FLAGMASK&(1U<<(*s-' '))); s++)
+			fl |= 1U << (*s - ' ');
 
 		/* Read field width */
 		if (*s=='*') {
diff --git a/src/stdio/vfwprintf.c b/src/stdio/vfwprintf.c
index 0adf0b7a..432f5e4a 100644
--- a/src/stdio/vfwprintf.c
+++ b/src/stdio/vfwprintf.c
@@ -12,12 +12,12 @@
 /* Convenient bit representation for modifier flags, which all fall
  * within 31 codepoints of the space character. */
 
-#define ALT_FORM   (1U<<'#'-' ')
-#define ZERO_PAD   (1U<<'0'-' ')
-#define LEFT_ADJ   (1U<<'-'-' ')
-#define PAD_POS    (1U<<' '-' ')
-#define MARK_POS   (1U<<'+'-' ')
-#define GROUPED    (1U<<'\''-' ')
+#define ALT_FORM   (1U << ('#' - ' '))
+#define ZERO_PAD   (1U << ('0' - ' '))
+#define LEFT_ADJ   (1U << ('-' - ' '))
+#define PAD_POS    (1U << (' ' - ' '))
+#define MARK_POS   (1U << ('+' - ' '))
+#define GROUPED    (1U << ('\'' - ' '))
 
 #define FLAGMASK (ALT_FORM|ZERO_PAD|LEFT_ADJ|PAD_POS|MARK_POS|GROUPED)
 
@@ -184,8 +184,8 @@ static int wprintf_core(FILE *f, const wchar_t *fmt, va_list *ap, union arg *nl_
 		}
 
 		/* Read modifier flags */
-		for (fl=0; (unsigned)*s-' '<32 && (FLAGMASK&(1U<<*s-' ')); s++)
-			fl |= 1U<<*s-' ';
+		for (fl=0; ((unsigned)*s-' ')<32 && (FLAGMASK&(1U<<(*s-' '))); s++)
+			fl |= 1U << (*s - ' ');
 
 		/* Read field width */
 		if (*s=='*') {
diff --git a/src/thread/pthread_cancel.c b/src/thread/pthread_cancel.c
index 2f9d5e97..9cc187f3 100644
--- a/src/thread/pthread_cancel.c
+++ b/src/thread/pthread_cancel.c
@@ -56,7 +56,7 @@ static void cancel_handler(int sig, siginfo_t *si, void *ctx)
 
 	_sigaddset(&uc->uc_sigmask, SIGCANCEL);
 
-	if (self->cancelasync || pc >= (uintptr_t)__cp_begin && pc < (uintptr_t)__cp_end) {
+	if (self->cancelasync || (pc >= (uintptr_t)__cp_begin && pc < (uintptr_t)__cp_end)) {
 		uc->uc_mcontext.MC_PC = (uintptr_t)__cp_cancel;
 #ifdef CANCEL_GOT
 		uc->uc_mcontext.MC_GOT = CANCEL_GOT;
-- 
2.19.1


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix many compiler warnings

[Alexander Monakov]

On Sat, 10 Nov 2018, 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).

By building the library twice (with and without your patch) and checking whether
generated binary code remains the same, you can test if your patch changes
meaning of the code.

(although you'd need to make sure that differences in debug info, if any appear
due to your changes, are discounted, for example by invoking 'strip' first)

Alexander

Re: [PATCH] Fix many compiler warnings

[Szabolcs Nagy]

* Sedrubal <dev@sedrubal.de> [2018-11-10 17:48:23 +0100]:
> 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).

suppress the warnings, you will just introduce bugs

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

e.g. this is wrong

Re: [PATCH] Fix many compiler warnings

[Markus Wichmann]

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

Re: [PATCH] Fix many compiler warnings

[Rich Felker]

On Sat, Nov 10, 2018 at 06:59:32PM +0100, Markus Wichmann wrote:
> 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

musl's configure's --enable-warnings specifically enables (for gcc;
other compilers may vary) enables all that are actual constraint
violations or which are considered style mistakes for musl. However
it does part of this via -Wno-* added on to the end of -Wall, meaning
on some (probably newer) versions there may be on-by-default warnings
if --enable-warnings is not passed to configure. I think the defaults
here can and should be changed: always using the -Wno-* for ones we
don't want, and perhaps also always adding the ones we do want, or at
least making --enable-warnings the default, so that people don't just
add -Wall manually.

> platforms with multiple compilers is damn near impossible, anyway. (E.g.

Yes. One problem is that firm/cparser, and possibly also clang, has
a number of warnings on by default, and needs -w to start with a clean
slate of "none enabled". But with gcc, -w permanently disables all
warnings, even if a subsequent -W occurs. So getting this right for
all compilers is a pain.

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

Indeed the above is wrong, but this might be one of the few changes we
could actually consider doing (with it fixed to be right) since it's
in a public header and might impact building programs against musl.
Ideally compilers should not produce warnings for system headers,
though; when they are produced it usually indicates the user has
botched the install somehow or the build system has bugs like explicit
-I/usr/include (which badly breaks cross compiling and some other
setups).

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

Yeah. I'm fairly indifferent about this though.

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

Because, with x in the range 0-255, x<<16 and x<<8 cannot overflow,
but x<<24 can when x>=128.

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

It extracts a 10-bit value from a packed array of 10-bit values that
span 2 consecutive bytes at positions x*5/4 and x*5/4+1. Their offsets
within the bytes rotate around with a period of 8, shifting by 2 for
each successive slot, thus 2*x%8.

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

Mechanically it's correct, but if anything it makes it less readable.
There might be some intermediate form, like just parenthesizing the
RHS's of the <</>>, that would be more readable, but not by much. In
any case, changes like this should be motivated by "this change makes
something easier to understand", not by "the compiler printed a
warning".

Rich

Re: [PATCH] Fix many compiler warnings

[Sedrubal]

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

Thanks for your feedback.

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

You're right, I'm sorry. Unfortunately for me libc-test contains 5 errors for 
master, v1.1.20 and for master+my changes. So I'm not sure, if I broke 
something...

> In any case, changes like this should be motivated by "this change makes
> something easier to understand", not by "the compiler printed a
> warning".

Most likely you're right. But I thought, it's quite easy to fix these warnings 
and maybe fixing them helps people to understand the code. I've to confess that 
I'm not really familiar with C but right now I need tables like this [1] to 
understand your code... Maybe I'm not the only one...

> Indeed the above is wrong, but this might be one of the few changes we
> could actually consider doing (with it fixed to be right) since it's
> in a public header and might impact building programs against musl.

Yes, that's the reason, why I created this patch... I'm OK if you only apply 
some of the bits of the patch that belong to headers and that you like.

Below you can find an updated version of my patch.

Cheers,

Sedrubal

[1] https://en.wikipedia.org/wiki/
Operators_in_C_and_C%2B%2B#Operator_precedence

P.S.: Sorry for breaking the thread. I didn't subscribe to the mailing list 
before and I didn't manage to find out the message ID of the last mail in the 
thread, so I replied on my own mail...

---

diff --git a/include/byteswap.h b/include/byteswap.h
index 00b9df3c..5dad8351 100644
--- a/include/byteswap.h
+++ b/include/byteswap.h
@@ -6,17 +6,17 @@
 
 static __inline uint16_t __bswap_16(uint16_t __x)
 {
-	return __x<<8 | __x>>8;
+	return (__x << 8) | (__x >> 8);
 }
 
 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);
 }
 
 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..c38c1ffc 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);
 }
 
 static __inline uint64_t __bswap64(uint64_t __x)
 {
-	return __bswap32(__x)+0ULL<<32 | __bswap32(__x>>32);
+	return ((__bswap32(__x) + 0ULL) << 32) | __bswap32(__x >> 32);
 }
 
 #if __BYTE_ORDER == __LITTLE_ENDIAN
diff --git a/src/ctype/towctrans.c b/src/ctype/towctrans.c
index 8f681018..bd0136dd 100644
--- a/src/ctype/towctrans.c
+++ b/src/ctype/towctrans.c
@@ -259,12 +259,14 @@ static wchar_t __towcase(wchar_t wc, int lower)
 	 || (unsigned)wc - 0xabc0 <= 0xfeff-0xabc0)
 		return wc;
 	/* special case because the diff between upper/lower is too big */
-	if (lower && (unsigned)wc - 0x10a0 < 0x2e)
+	if (lower && (unsigned)wc - 0x10a0 < 0x2e) {
 		if (wc>0x10c5 && wc != 0x10c7 && wc != 0x10cd) return wc;
 		else return wc + 0x2d00 - 0x10a0;
-	if (!lower && (unsigned)wc - 0x2d00 < 0x26)
+	}
+	if (!lower && (unsigned)wc - 0x2d00 < 0x26) {
 		if (wc>0x2d25 && wc != 0x2d27 && wc != 0x2d2d) return wc;
 		else return wc + 0x10a0 - 0x2d00;
+	}
 	if (lower && (unsigned)wc - 0x13a0 < 0x50)
 		return wc + 0xab70 - 0x13a0;
 	if (!lower && (unsigned)wc - 0xab70 < 0x50)
diff --git a/src/internal/shgetc.c b/src/internal/shgetc.c
index ebd5fae7..c6aefed9 100644
--- a/src/internal/shgetc.c
+++ b/src/internal/shgetc.c
@@ -20,7 +20,7 @@ int __shgetc(FILE *f)
 {
 	int c;
 	off_t cnt = shcnt(f);
-	if (f->shlim && cnt >= f->shlim || (c=__uflow(f)) < 0) {
+	if ((f->shlim && cnt >= f->shlim) || (c=__uflow(f)) < 0) {
 		f->shcnt = f->buf - f->rpos + cnt;
 		f->shend = 0;
 		return EOF;
diff --git a/src/legacy/getpass.c b/src/legacy/getpass.c
index d51286c0..036a7859 100644
--- a/src/legacy/getpass.c
+++ b/src/legacy/getpass.c
@@ -27,7 +27,7 @@ char *getpass(const char *prompt)
 
 	l = read(fd, password, sizeof password);
 	if (l >= 0) {
-		if (l > 0 && password[l-1] == '\n' || l==sizeof password) l--;
+		if ((l > 0 && password[l-1] == '\n') || l==sizeof password) l--;
 		password[l] = 0;
 	}
 
diff --git a/src/locale/__mo_lookup.c b/src/locale/__mo_lookup.c
index d18ab774..818b7f9b 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;
 }
 
 const char *__mo_lookup(const void *p, size_t size, const char *s)
diff --git a/src/locale/dcngettext.c b/src/locale/dcngettext.c
index 8b891d00..30391ca6 100644
--- a/src/locale/dcngettext.c
+++ b/src/locale/dcngettext.c
@@ -176,7 +176,8 @@ 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;
+			map = __map_file(name, &map_size);
+			if (map) break;
 
 			/* Try dropping @mod, _YY, then both. */
 			if (alt_modlen) {
diff --git a/src/locale/iconv.c b/src/locale/iconv.c
index 3047c27b..c0992833 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];
 }
 
 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];
 }
 
@@ -495,7 +495,7 @@ size_t iconv(iconv_t cd, char **restrict in, size_t 
*restrict inb, char **restri
 			if (c >= 93 || d >= 94) {
 				c += (0xa1-0x81);
 				d += 0xa1;
-				if (c >= 93 || c>=0xc6-0x81 && d>0x52)
+				if (c >= 93 || ((c >= 0xc6 - 0x81) && d>0x52))
 					goto ilseq;
 				if (d-'A'<26) d = d-'A';
 				else if (d-'a'<26) d = d-'a'+26;
diff --git a/src/locale/locale_map.c b/src/locale/locale_map.c
index 2321bac0..8ee4641c 100644
--- a/src/locale/locale_map.c
+++ b/src/locale/locale_map.c
@@ -32,9 +32,9 @@ const struct __locale_map *__get_locale(int cat, const char 
*val)
 	size_t l, n;
 
 	if (!*val) {
-		(val = getenv("LC_ALL")) && *val ||
-		(val = getenv(envvars[cat])) && *val ||
-		(val = getenv("LANG")) && *val ||
+		((val = getenv("LC_ALL")) && *val) ||
+		((val = getenv(envvars[cat])) && *val) ||
+		((val = getenv("LANG")) && *val) ||
 		(val = "C.UTF-8");
 	}
 
diff --git a/src/multibyte/c16rtomb.c b/src/multibyte/c16rtomb.c
index 39ca3758..5ebfbc29 100644
--- a/src/multibyte/c16rtomb.c
+++ b/src/multibyte/c16rtomb.c
@@ -15,7 +15,7 @@ size_t c16rtomb(char *restrict s, char16_t c16, mbstate_t 
*restrict ps)
 	}
 
 	if (!*x && c16 - 0xd800u < 0x400) {
-		*x = c16 - 0xd7c0 << 10;
+		*x = (c16 - 0xd7c0) << 10;
 		return 0;
 	}
 
diff --git a/src/network/dn_skipname.c b/src/network/dn_skipname.c
index d54c2e5d..44f8e68c 100644
--- a/src/network/dn_skipname.c
+++ b/src/network/dn_skipname.c
@@ -3,10 +3,12 @@
 int dn_skipname(const unsigned char *s, const unsigned char *end)
 {
 	const unsigned char *p;
-	for (p=s; p<end; p++)
+	for (p=s; p<end; p++) {
 		if (!*p) return p-s+1;
-		else if (*p>=192)
+		else if (*p>=192) {
 			if (p+1<end) return p-s+2;
 			else break;
+		}
+	}
 	return -1;
 }
diff --git a/src/passwd/getgr_a.c b/src/passwd/getgr_a.c
index afeb1ece..5605b4b6 100644
--- a/src/passwd/getgr_a.c
+++ b/src/passwd/getgr_a.c
@@ -33,8 +33,8 @@ int __getgr_a(const char *name, gid_t gid, struct group *gr, 
char **buf, size_t
 	}
 
 	while (!(rv = __getgrent_a(f, gr, buf, size, mem, nmem, res)) && *res) {
-		if (name && !strcmp(name, (*res)->gr_name)
-		|| !name && (*res)->gr_gid == gid) {
+		if ((name && !strcmp(name, (*res)->gr_name))
+		|| (!name && (*res)->gr_gid == gid)) {
 			break;
 		}
 	}
@@ -149,8 +149,8 @@ int __getgr_a(const char *name, gid_t gid, struct group 
*gr, char **buf, size_t
 			goto cleanup_f;
 		}
 
-		if (name && strcmp(name, gr->gr_name)
-		|| !name && gid != gr->gr_gid) {
+		if ((name && strcmp(name, gr->gr_name))
+		|| (!name && gid != gr->gr_gid)) {
 			rv = EIO;
 			goto cleanup_f;
 		}
diff --git a/src/passwd/getpw_a.c b/src/passwd/getpw_a.c
index 15a70c03..e1406319 100644
--- a/src/passwd/getpw_a.c
+++ b/src/passwd/getpw_a.c
@@ -34,8 +34,8 @@ int __getpw_a(const char *name, uid_t uid, struct passwd 
*pw, char **buf, size_t
 	}
 
 	while (!(rv = __getpwent_a(f, pw, buf, size, res)) && *res) {
-		if (name && !strcmp(name, (*res)->pw_name)
-		|| !name && (*res)->pw_uid == uid)
+		if ((name && !strcmp(name, (*res)->pw_name))
+		|| (!name && (*res)->pw_uid == uid))
 			break;
 	}
 	fclose(f);
@@ -122,8 +122,8 @@ int __getpw_a(const char *name, uid_t uid, struct passwd 
*pw, char **buf, size_t
 			goto cleanup_f;
 		}
 
-		if (name && strcmp(name, pw->pw_name)
-		|| !name && uid != pw->pw_uid) {
+		if ((name && strcmp(name, pw->pw_name))
+		|| (!name && uid != pw->pw_uid)) {
 			rv = EIO;
 			goto cleanup_f;
 		}
diff --git a/src/prng/__rand48_step.c b/src/prng/__rand48_step.c
index 94703d07..745fb0ee 100644
--- a/src/prng/__rand48_step.c
+++ b/src/prng/__rand48_step.c
@@ -4,8 +4,8 @@
 uint64_t __rand48_step(unsigned short *xi, unsigned short *lc)
 {
 	uint64_t a, x;
-	x = xi[0] | xi[1]+0U<<16 | xi[2]+0ULL<<32;
-	a = lc[0] | lc[1]+0U<<16 | lc[2]+0ULL<<32;
+	x = xi[0] | ((xi[1] + 0U) << 16) | ((xi[2] + 0ULL) << 32);
+	a = lc[0] | ((lc[1] + 0U) << 16) | ((lc[2] + 0ULL) << 32);
 	x = a*x + lc[3];
 	xi[0] = x;
 	xi[1] = x>>16;
diff --git a/src/regex/glob.c b/src/regex/glob.c
index aa1c6a44..72c9b5dc 100644
--- a/src/regex/glob.c
+++ b/src/regex/glob.c
@@ -142,7 +142,7 @@ static int do_glob(char *buf, size_t pos, int type, char 
*pat, int flags, int (*
 		/* With GLOB_PERIOD, don't allow matching . or .. unless
 		 * fnmatch would match them with FNM_PERIOD rules in effect. */
 		if (p2 && (flags & GLOB_PERIOD) && de->d_name[0]=='.'
-		    && (!de->d_name[1] || de->d_name[1]=='.' && !de->d_name[2])
+		    && (!de->d_name[1] || (de->d_name[1]=='.' && !de->d_name[2]))
 		    && fnmatch(pat, de->d_name, fnm_flags | FNM_PERIOD))
 			continue;
 
diff --git a/src/select/select.c b/src/select/select.c
index 02fd75c3..c913fe6b 100644
--- a/src/select/select.c
+++ b/src/select/select.c
@@ -16,7 +16,7 @@ int select(int n, fd_set *restrict rfds, fd_set *restrict 
wfds, fd_set *restrict
 			return __syscall_ret(-EINVAL);
 		time_t extra_secs = tv->tv_usec / 1000000;
 		ts.tv_nsec = tv->tv_usec % 1000000 * 1000;
-		const time_t max_time = (1ULL<<8*sizeof(time_t)-1)-1;
+		const time_t max_time = (1ULL << (8 * sizeof(time_t) - 1)) - 1;
 		ts.tv_sec = extra_secs > max_time - tv->tv_sec ?
 			max_time : tv->tv_sec + extra_secs;
 	}
diff --git a/src/stdio/getc.h b/src/stdio/getc.h
index e24f9905..7a4643cd 100644
--- a/src/stdio/getc.h
+++ b/src/stdio/getc.h
@@ -16,7 +16,7 @@ static int locking_getc(FILE *f)
 static inline int do_getc(FILE *f)
 {
 	int l = f->lock;
-	if (l < 0 || l && (l & ~MAYBE_WAITERS) == __pthread_self()->tid)
+	if (l < 0 || (l && (l & ~MAYBE_WAITERS) == __pthread_self()->tid))
 		return getc_unlocked(f);
 	return locking_getc(f);
 }
diff --git a/src/stdio/putc.h b/src/stdio/putc.h
index 2014c4ec..2d7dce16 100644
--- a/src/stdio/putc.h
+++ b/src/stdio/putc.h
@@ -16,7 +16,7 @@ static int locking_putc(int c, FILE *f)
 static inline int do_putc(int c, FILE *f)
 {
 	int l = f->lock;
-	if (l < 0 || l && (l & ~MAYBE_WAITERS) == __pthread_self()->tid)
+	if (l < 0 || (l && (l & ~MAYBE_WAITERS) == __pthread_self()->tid))
 		return putc_unlocked(c, f);
 	return locking_putc(c, f);
 }
diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c
index 9b961e7f..695f8ed5 100644
--- a/src/stdio/vfprintf.c
+++ b/src/stdio/vfprintf.c
@@ -19,12 +19,12 @@
 /* Convenient bit representation for modifier flags, which all fall
  * within 31 codepoints of the space character. */
 
-#define ALT_FORM   (1U<<'#'-' ')
-#define ZERO_PAD   (1U<<'0'-' ')
-#define LEFT_ADJ   (1U<<'-'-' ')
-#define PAD_POS    (1U<<' '-' ')
-#define MARK_POS   (1U<<'+'-' ')
-#define GROUPED    (1U<<'\''-' ')
+#define ALT_FORM   (1U << ('#' - ' '))
+#define ZERO_PAD   (1U << ('0' - ' '))
+#define LEFT_ADJ   (1U << ('-' - ' '))
+#define PAD_POS    (1U << (' ' - ' '))
+#define MARK_POS   (1U << ('+' - ' '))
+#define GROUPED    (1U << ('\'' - ' '))
 
 #define FLAGMASK (ALT_FORM|ZERO_PAD|LEFT_ADJ|PAD_POS|MARK_POS|GROUPED)
 
@@ -471,8 +471,8 @@ static int printf_core(FILE *f, const char *fmt, va_list 
*ap, union arg *nl_arg,
 		}
 
 		/* Read modifier flags */
-		for (fl=0; (unsigned)*s-' '<32 && (FLAGMASK&(1U<<*s-' ')); s++)
-			fl |= 1U<<*s-' ';
+		for (fl=0; ((unsigned)*s-' ')<32 && (FLAGMASK&(1U<<(*s-' '))); s++)
+			fl |= 1U << (*s - ' ');
 
 		/* Read field width */
 		if (*s=='*') {
diff --git a/src/stdio/vfwprintf.c b/src/stdio/vfwprintf.c
index 0adf0b7a..432f5e4a 100644
--- a/src/stdio/vfwprintf.c
+++ b/src/stdio/vfwprintf.c
@@ -12,12 +12,12 @@
 /* Convenient bit representation for modifier flags, which all fall
  * within 31 codepoints of the space character. */
 
-#define ALT_FORM   (1U<<'#'-' ')
-#define ZERO_PAD   (1U<<'0'-' ')
-#define LEFT_ADJ   (1U<<'-'-' ')
-#define PAD_POS    (1U<<' '-' ')
-#define MARK_POS   (1U<<'+'-' ')
-#define GROUPED    (1U<<'\''-' ')
+#define ALT_FORM   (1U << ('#' - ' '))
+#define ZERO_PAD   (1U << ('0' - ' '))
+#define LEFT_ADJ   (1U << ('-' - ' '))
+#define PAD_POS    (1U << (' ' - ' '))
+#define MARK_POS   (1U << ('+' - ' '))
+#define GROUPED    (1U << ('\'' - ' '))
 
 #define FLAGMASK (ALT_FORM|ZERO_PAD|LEFT_ADJ|PAD_POS|MARK_POS|GROUPED)
 
@@ -184,8 +184,8 @@ static int wprintf_core(FILE *f, const wchar_t *fmt, 
va_list *ap, union arg *nl_
 		}
 
 		/* Read modifier flags */
-		for (fl=0; (unsigned)*s-' '<32 && (FLAGMASK&(1U<<*s-' ')); s++)
-			fl |= 1U<<*s-' ';
+		for (fl=0; ((unsigned)*s-' ')<32 && (FLAGMASK&(1U<<(*s-' '))); s++)
+			fl |= 1U << (*s - ' ');
 
 		/* Read field width */
 		if (*s=='*') {
diff --git a/src/thread/pthread_cancel.c b/src/thread/pthread_cancel.c
index 2f9d5e97..9cc187f3 100644
--- a/src/thread/pthread_cancel.c
+++ b/src/thread/pthread_cancel.c
@@ -56,7 +56,7 @@ static void cancel_handler(int sig, siginfo_t *si, void 
*ctx)
 
 	_sigaddset(&uc->uc_sigmask, SIGCANCEL);
 
-	if (self->cancelasync || pc >= (uintptr_t)__cp_begin && pc < 
(uintptr_t)__cp_end) {
+	if (self->cancelasync || (pc >= (uintptr_t)__cp_begin && pc < 
(uintptr_t)__cp_end)) {
 		uc->uc_mcontext.MC_PC = (uintptr_t)__cp_cancel;
 #ifdef CANCEL_GOT
 		uc->uc_mcontext.MC_GOT = CANCEL_GOT;

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Re: [PATCH] Fix many compiler warnings

[Szabolcs Nagy]

* Sedrubal <dev@sedrubal.de> [2018-11-13 01:15:11 +0100]:
> > Well, hope belongs in the church. Whether or not you introduced bugs is
> > a thing you can test for. There is libc-test.
> 
> You're right, I'm sorry. Unfortunately for me libc-test contains 5 errors for 
> master, v1.1.20 and for master+my changes. So I'm not sure, if I broke 
> something...
> 

you don't need to run libc-test.

you have to ensure that before and after the patch the
exact same stripped libc binaries are generated and point
this out in the commit message.

(in principle register allocation, instruction scheduling
etc can change, in practice that should not happen if you
get all parenthesis right and use the same toolchain.

not all header files are used in libc code so you could
write further tests for them or compile code using them
if you want to make sure everything works, but that's
not critical.)