mailing list of musl libc
 help / color / mirror / code / Atom feed
* [PATCH 1/5] fix warning dangling-else
@ 2019-07-22 18:07 Issam Maghni
  2019-07-22 18:07 ` [PATCH 2/5] fix warning logical-op-parentheses Issam Maghni
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Issam Maghni @ 2019-07-22 18:07 UTC (permalink / raw)
  To: musl; +Cc: Issam Maghni

Signed-off-by: Issam Maghni <me@concati.me>
---
 src/ctype/towctrans.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/5] fix warning logical-op-parentheses
  2019-07-22 18:07 [PATCH 1/5] fix warning dangling-else Issam Maghni
@ 2019-07-22 18:07 ` Issam Maghni
  2019-07-22 18:07 ` [PATCH 3/5] fix warning bitwise-op-parentheses Issam Maghni
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Issam Maghni @ 2019-07-22 18:07 UTC (permalink / raw)
  To: musl; +Cc: Issam Maghni

Signed-off-by: Issam Maghni <me@concati.me>
---
 src/internal/shgetc.c       | 2 +-
 src/legacy/getpass.c        | 2 +-
 src/locale/iconv.c          | 2 +-
 src/locale/locale_map.c     | 6 +++---
 src/passwd/getgr_a.c        | 8 ++++----
 src/passwd/getpw_a.c        | 8 ++++----
 src/regex/glob.c            | 8 ++++----
 src/stdio/getc.h            | 2 +-
 src/stdio/putc.h            | 2 +-
 src/thread/pthread_cancel.c | 2 +-
 10 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/src/internal/shgetc.c b/src/internal/shgetc.c
index a4a9c633..b458d93e 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 = f->rpos;
 		f->shlim = -1;
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/iconv.c b/src/locale/iconv.c
index 3047c27b..ecb57bc2 100644
--- a/src/locale/iconv.c
+++ b/src/locale/iconv.c
@@ -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/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/regex/glob.c b/src/regex/glob.c
index aa1c6a44..3ce69913 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;
 
@@ -189,7 +189,7 @@ int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, i
 	size_t offs = (flags & GLOB_DOOFFS) ? g->gl_offs : 0;
 	int error = 0;
 	char buf[PATH_MAX];
-	
+
 	if (!errfunc) errfunc = ignore_err;
 
 	if (!(flags & GLOB_APPEND)) {
@@ -210,7 +210,7 @@ int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, i
 		freelist(&head);
 		return error;
 	}
-	
+
 	for (cnt=0, tail=head.next; tail; tail=tail->next, cnt++);
 	if (!cnt) {
 		if (flags & GLOB_NOCHECK) {
@@ -246,7 +246,7 @@ int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, i
 
 	if (!(flags & GLOB_NOSORT))
 		qsort(g->gl_pathv+offs, cnt, sizeof(char *), sort);
-	
+
 	return error;
 }
 
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/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.22.0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/5] fix warning bitwise-op-parentheses
  2019-07-22 18:07 [PATCH 1/5] fix warning dangling-else Issam Maghni
  2019-07-22 18:07 ` [PATCH 2/5] fix warning logical-op-parentheses Issam Maghni
@ 2019-07-22 18:07 ` Issam Maghni
  2019-07-22 18:07 ` [PATCH 4/5] fix warning shift-op-parentheses Issam Maghni
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Issam Maghni @ 2019-07-22 18:07 UTC (permalink / raw)
  To: musl; +Cc: Issam Maghni

Signed-off-by: Issam Maghni <me@concati.me>
---
 include/byteswap.h       | 2 +-
 include/endian.h         | 2 +-
 src/locale/__mo_lookup.c | 2 +-
 src/locale/iconv.c       | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/byteswap.h b/include/byteswap.h
index 00b9df3c..861c3aa1 100644
--- a/include/byteswap.h
+++ b/include/byteswap.h
@@ -11,7 +11,7 @@ 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)
diff --git a/include/endian.h b/include/endian.h
index 1bd44451..60716508 100644
--- a/include/endian.h
+++ b/include/endian.h
@@ -29,7 +29,7 @@ static __inline uint16_t __bswap16(uint16_t __x)
 
 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)
diff --git a/src/locale/__mo_lookup.c b/src/locale/__mo_lookup.c
index d18ab774..fac204e2 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/iconv.c b/src/locale/iconv.c
index ecb57bc2..28c7709e 100644
--- a/src/locale/iconv.c
+++ b/src/locale/iconv.c
@@ -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];
 }
 
-- 
2.22.0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 4/5] fix warning shift-op-parentheses
  2019-07-22 18:07 [PATCH 1/5] fix warning dangling-else Issam Maghni
  2019-07-22 18:07 ` [PATCH 2/5] fix warning logical-op-parentheses Issam Maghni
  2019-07-22 18:07 ` [PATCH 3/5] fix warning bitwise-op-parentheses Issam Maghni
@ 2019-07-22 18:07 ` Issam Maghni
  2019-07-22 18:07 ` [PATCH 5/5] fix warning parentheses Issam Maghni
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Issam Maghni @ 2019-07-22 18:07 UTC (permalink / raw)
  To: musl; +Cc: Issam Maghni

Signed-off-by: Issam Maghni <me@concati.me>
---
 include/byteswap.h       |  2 +-
 include/endian.h         |  2 +-
 src/locale/iconv.c       |  4 ++--
 src/multibyte/c16rtomb.c |  2 +-
 src/prng/__rand48_step.c |  4 ++--
 src/stdio/vfprintf.c     | 18 +++++++++---------
 src/stdio/vfwprintf.c    | 16 ++++++++--------
 7 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/include/byteswap.h b/include/byteswap.h
index 861c3aa1..928be2b3 100644
--- a/include/byteswap.h
+++ b/include/byteswap.h
@@ -16,7 +16,7 @@ static __inline uint32_t __bswap_32(uint32_t __x)
 
 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 60716508..88c3347b 100644
--- a/include/endian.h
+++ b/include/endian.h
@@ -34,7 +34,7 @@ static __inline uint32_t __bswap32(uint32_t __x)
 
 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/locale/iconv.c b/src/locale/iconv.c
index 28c7709e..c3f790b1 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];
 }
 
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/prng/__rand48_step.c b/src/prng/__rand48_step.c
index 94703d07..4e4e9ab6 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/stdio/vfprintf.c b/src/stdio/vfprintf.c
index 9b961e7f..ac89abc6 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)
 
@@ -341,7 +341,7 @@ static int fmt_fp(FILE *f, long double y, int w, int p, int fl, int t)
 		if (z>d+1) z=d+1;
 	}
 	for (; z>a && !z[-1]; z--);
-	
+
 	if ((t|32)=='g') {
 		if (!p) p++;
 		if (p>e && e>=-4) {
@@ -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..a30be6ee 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=='*') {
-- 
2.22.0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 5/5] fix warning parentheses
  2019-07-22 18:07 [PATCH 1/5] fix warning dangling-else Issam Maghni
                   ` (2 preceding siblings ...)
  2019-07-22 18:07 ` [PATCH 4/5] fix warning shift-op-parentheses Issam Maghni
@ 2019-07-22 18:07 ` Issam Maghni
  2019-07-22 18:50 ` [PATCH 1/5] fix warning dangling-else A. Wilcox
  2019-07-22 20:50 ` Rich Felker
  5 siblings, 0 replies; 11+ messages in thread
From: Issam Maghni @ 2019-07-22 18:07 UTC (permalink / raw)
  To: musl; +Cc: Issam Maghni

Signed-off-by: Issam Maghni <me@concati.me>
---
 src/locale/dcngettext.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/locale/dcngettext.c b/src/locale/dcngettext.c
index 4c304393..3a10e42f 100644
--- a/src/locale/dcngettext.c
+++ b/src/locale/dcngettext.c
@@ -81,7 +81,7 @@ char *bindtextdomain(const char *domainname, const char *dirname)
 	}
 
 	UNLOCK(lock);
-	
+
 	return (char *)p->dirname;
 }
 
@@ -178,7 +178,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) {
-- 
2.22.0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] fix warning dangling-else
  2019-07-22 18:07 [PATCH 1/5] fix warning dangling-else Issam Maghni
                   ` (3 preceding siblings ...)
  2019-07-22 18:07 ` [PATCH 5/5] fix warning parentheses Issam Maghni
@ 2019-07-22 18:50 ` A. Wilcox
  2019-07-22 20:50 ` Rich Felker
  5 siblings, 0 replies; 11+ messages in thread
From: A. Wilcox @ 2019-07-22 18:50 UTC (permalink / raw)
  To: musl


[-- Attachment #1.1: Type: text/plain, Size: 1389 bytes --]

On 07/22/19 13:07, Issam Maghni wrote:
> Signed-off-by: Issam Maghni <me@concati.me>
> ---
>  src/ctype/towctrans.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> 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)
> 


I know none of these changes have a chance of making it in, but this one
specifically *does* genuinely irk me and I *would* love to see this one
actually merged to musl.

--arw


-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] fix warning dangling-else
  2019-07-22 18:07 [PATCH 1/5] fix warning dangling-else Issam Maghni
                   ` (4 preceding siblings ...)
  2019-07-22 18:50 ` [PATCH 1/5] fix warning dangling-else A. Wilcox
@ 2019-07-22 20:50 ` Rich Felker
  2019-07-23  2:31   ` Fangrui Song
  5 siblings, 1 reply; 11+ messages in thread
From: Rich Felker @ 2019-07-22 20:50 UTC (permalink / raw)
  To: musl

On Mon, Jul 22, 2019 at 02:07:36PM -0400, Issam Maghni wrote:
> Signed-off-by: Issam Maghni <me@concati.me>
> ---
>  src/ctype/towctrans.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> 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)
> -- 
> 2.22.0

To clarify A. Wilcox's comment about "no chance of making it in", the
coding style used in musl explicitly does not attempt to conform to
the style rules that the warnings in this patch series are about. So
there are questions of what the patches are attempting to address --
is the goal to make clang stop spamming warnings, or to improve
readability, or some mix of the two? If they were applied, would you
be unhappy if the same warnings reappeared a few weeks layer due to
new code somewhere else (in which case the request is really about a
*policy* change, rather than an immediate code change)? Etc.

I'm fairly neutral about the change above in patch 1, but opposed to
most of the others. To me, visually, multiple levels of parentheses
are hard to read. Much harder than understanding operator precedence.
musl does make use of operator precedence, and assumes the reader is
aware of it. In lots of places where precedence is relied upon,
omission of spacing between some operators/operands is used as a hint
to the reader of how the expression groups. In other places
(especially &&/|| where it feels unnatural) it's usually not.

Applying gratuitous style change commits is not without cost. Any bug
fixes made after the style change commit will not apply to older
versions of the software without manual fixups. Of course this happens
for functional changes too, but in that case at least the change was
well-motivated rather than being gratuitous. In the case of patch 1
here, there's actually a pending replacement implementation for the
whole file. I've held back on making the replacement because there
were still some open questions about tuning it and it's considerably
(a few kB) larger despite being much faster and more maintainable. So
it probably doesn't make sense to apply a style change here now even
if it were agreed to be desirable.


What would really be much more welcome is a fix in configure for the
default warnings options. Right now, if you use --enable-warnings, it
enabled -Wall then disables known-unwanted ones by name; it's assumed
that, without any -W options, the only warnings on will be ones for
exceptionally egregious things that warrant the compiler enabling them
by default. This was always true for GCC, but not for clang or
cparser/firm.

Just always doing the -Wno-* part would help somewhat, but it won't
keep up with new on-by-default warnings of clang, and if
--enable-warnings is used, it won't keep up with new additions to
-Wall that might not be wanted.

For clang and cparser, adding -w to CFLAGS would let us start with a
"clean slate" and add only the warnings we want. But for gcc, -w
overrides any -W options, even subsequent ones, so we have to avoid
passing -w if the compiler is real gcc.

I've explored in the past getting rid of -Wall from --enable-warnings
and instead explicitly adding each warning option that's definite or
near-sure UB or hard constraint violations, rather than a style
warning. This is probably the right course of action.

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] fix warning dangling-else
  2019-07-22 20:50 ` Rich Felker
@ 2019-07-23  2:31   ` Fangrui Song
  2019-07-23  3:38     ` Rich Felker
  0 siblings, 1 reply; 11+ messages in thread
From: Fangrui Song @ 2019-07-23  2:31 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

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

On 2019-07-22, Rich Felker wrote:
>On Mon, Jul 22, 2019 at 02:07:36PM -0400, Issam Maghni wrote:
>> Signed-off-by: Issam Maghni <me@concati.me>
>> ---
>>  src/ctype/towctrans.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> 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)
>> --
>> 2.22.0
>
>To clarify A. Wilcox's comment about "no chance of making it in", the
>coding style used in musl explicitly does not attempt to conform to
>the style rules that the warnings in this patch series are about. So
>there are questions of what the patches are attempting to address --
>is the goal to make clang stop spamming warnings, or to improve
>readability, or some mix of the two? If they were applied, would you
>be unhappy if the same warnings reappeared a few weeks layer due to
>new code somewhere else (in which case the request is really about a
>*policy* change, rather than an immediate code change)? Etc.
>
>I'm fairly neutral about the change above in patch 1, but opposed to
>most of the others. To me, visually, multiple levels of parentheses
>are hard to read. Much harder than understanding operator precedence.
>musl does make use of operator precedence, and assumes the reader is
>aware of it. In lots of places where precedence is relied upon,
>omission of spacing between some operators/operands is used as a hint
>to the reader of how the expression groups. In other places
>(especially &&/|| where it feels unnatural) it's usually not.
>
>Applying gratuitous style change commits is not without cost. Any bug
>fixes made after the style change commit will not apply to older
>versions of the software without manual fixups. Of course this happens
>for functional changes too, but in that case at least the change was
>well-motivated rather than being gratuitous. In the case of patch 1
>here, there's actually a pending replacement implementation for the
>whole file. I've held back on making the replacement because there
>were still some open questions about tuning it and it's considerably
>(a few kB) larger despite being much faster and more maintainable. So
>it probably doesn't make sense to apply a style change here now even
>if it were agreed to be desirable.
>
>
>What would really be much more welcome is a fix in configure for the
>default warnings options. Right now, if you use --enable-warnings, it
>enabled -Wall then disables known-unwanted ones by name; it's assumed
>that, without any -W options, the only warnings on will be ones for
>exceptionally egregious things that warrant the compiler enabling them
>by default. This was always true for GCC, but not for clang or
>cparser/firm.
>
>Just always doing the -Wno-* part would help somewhat, but it won't
>keep up with new on-by-default warnings of clang, and if
>--enable-warnings is used, it won't keep up with new additions to
>-Wall that might not be wanted.
>
>For clang and cparser, adding -w to CFLAGS would let us start with a
>"clean slate" and add only the warnings we want. But for gcc, -w
>overrides any -W options, even subsequent ones, so we have to avoid
>passing -w if the compiler is real gcc.
>
>I've explored in the past getting rid of -Wall from --enable-warnings
>and instead explicitly adding each warning option that's definite or
>near-sure UB or hard constraint violations, rather than a style
>warning. This is probably the right course of action.
>
>Rich

With the attached patch, gcc has just some warnings in src/ctype/towctrans.c

[-Wdangling-else]
  supposedly it will be address soon: "In the case of patch 1 here, there's actually a pending replacement implementation for the whole file."

clang has a few more:

% grep -o '\[-.*\]' /tmp/clang.log | sort | uniq -c
 4 [-Wdangling-else]
10 [-Wignored-attributes]
     they are all in the form of `weak_alias(statfs, statfs64)`.
     these warnings will go away when the lfs64 things are fixed.
18 [-Wunknown-pragmas]
     src/math/fmal.c:167:15: warning: pragma STDC FENV_ACCESS ON is not supported, ignoring pragma [-Wunknown-pragmas]
             #pragma STDC FENV_ACCESS ON
     There is a long-standing bug https://bugs.llvm.org/show_bug.cgi?id=8100
     "[llvm-dev] [cfe-dev] Why is #pragma STDC FENV_ACCESS not supported?" was a 2018 discussion on this topic.

[-Wdangling-else] and [-Wignored-attributes] will go away soon.

[-- Attachment #2: musl.patch --]
[-- Type: text/x-diff, Size: 1343 bytes --]

From bf24cf2d5717505b5c880d2eb6714789f86a902c Mon Sep 17 00:00:00 2001
From: Fangrui Song <i@maskray.me>
Date: Tue, 23 Jul 2019 02:02:47 +0000
Subject: [PATCH] disable some known-unwanted but enabled-by-default warnings
 in clang

the known-unwanted -Wstring-plus-int and the warning group -Wparentheses
are enabled by default in clang. adjust CFLAGS_AUTO to disable these
warnings whether or not --enable-warnings is specified.
---
 configure | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 86801281..7f63a873 100755
--- a/configure
+++ b/configure
@@ -514,7 +514,6 @@ test "$cc_family" = clang && tryflag CFLAGS_AUTO -Qunused-arguments
 
 if test "x$warnings" = xyes ; then
 tryflag CFLAGS_AUTO -Wall
-tryflag CFLAGS_AUTO -Wno-parentheses
 tryflag CFLAGS_AUTO -Wno-uninitialized
 tryflag CFLAGS_AUTO -Wno-missing-braces
 tryflag CFLAGS_AUTO -Wno-unused-value
@@ -522,6 +521,9 @@ tryflag CFLAGS_AUTO -Wno-unused-but-set-variable
 tryflag CFLAGS_AUTO -Wno-unknown-pragmas
 tryflag CFLAGS_AUTO -Wno-pointer-to-int-cast
 fi
+tryflag CFLAGS_AUTO -Wno-string-plus-int
+tryflag CFLAGS_AUTO -Wno-parentheses
+tryflag CFLAGS_AUTO -Wdangling-else
 
 # Determine if the compiler produces position-independent code (PIC)
 # by default. If so, we don't need to compile separate object files
-- 
2.22.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] fix warning dangling-else
  2019-07-23  2:31   ` Fangrui Song
@ 2019-07-23  3:38     ` Rich Felker
  2019-07-23  4:06       ` Fangrui Song
  0 siblings, 1 reply; 11+ messages in thread
From: Rich Felker @ 2019-07-23  3:38 UTC (permalink / raw)
  To: musl

On Tue, Jul 23, 2019 at 02:31:24AM +0000, Fangrui Song wrote:
> With the attached patch, gcc has just some warnings in src/ctype/towctrans.c
> 
> [-Wdangling-else]
>  supposedly it will be address soon: "In the case of patch 1 here,
>  there's actually a pending replacement implementation for the whole
>  file."
> 
> clang has a few more:
> 
> % grep -o '\[-.*\]' /tmp/clang.log | sort | uniq -c
> 4 [-Wdangling-else]
> 10 [-Wignored-attributes]
>     they are all in the form of `weak_alias(statfs, statfs64)`.
>     these warnings will go away when the lfs64 things are fixed.
> 18 [-Wunknown-pragmas]
>     src/math/fmal.c:167:15: warning: pragma STDC FENV_ACCESS ON is not supported, ignoring pragma [-Wunknown-pragmas]
>             #pragma STDC FENV_ACCESS ON
>     There is a long-standing bug https://bugs.llvm.org/show_bug.cgi?id=8100
>     "[llvm-dev] [cfe-dev] Why is #pragma STDC FENV_ACCESS not supported?" was a 2018 discussion on this topic.
> 
> [-Wdangling-else] and [-Wignored-attributes] will go away soon.

> From bf24cf2d5717505b5c880d2eb6714789f86a902c Mon Sep 17 00:00:00 2001
> From: Fangrui Song <i@maskray.me>
> Date: Tue, 23 Jul 2019 02:02:47 +0000
> Subject: [PATCH] disable some known-unwanted but enabled-by-default warnings
>  in clang
> 
> the known-unwanted -Wstring-plus-int and the warning group -Wparentheses
> are enabled by default in clang. adjust CFLAGS_AUTO to disable these
> warnings whether or not --enable-warnings is specified.
> ---
>  configure | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 86801281..7f63a873 100755
> --- a/configure
> +++ b/configure
> @@ -514,7 +514,6 @@ test "$cc_family" = clang && tryflag CFLAGS_AUTO -Qunused-arguments
>  
>  if test "x$warnings" = xyes ; then
>  tryflag CFLAGS_AUTO -Wall
> -tryflag CFLAGS_AUTO -Wno-parentheses
>  tryflag CFLAGS_AUTO -Wno-uninitialized
>  tryflag CFLAGS_AUTO -Wno-missing-braces
>  tryflag CFLAGS_AUTO -Wno-unused-value
> @@ -522,6 +521,9 @@ tryflag CFLAGS_AUTO -Wno-unused-but-set-variable
>  tryflag CFLAGS_AUTO -Wno-unknown-pragmas
>  tryflag CFLAGS_AUTO -Wno-pointer-to-int-cast
>  fi
> +tryflag CFLAGS_AUTO -Wno-string-plus-int
> +tryflag CFLAGS_AUTO -Wno-parentheses
> +tryflag CFLAGS_AUTO -Wdangling-else

Why is the patch adding a test to *enable* a warning outside of the
--enable-warnings case? The -Wno's here make sense -- maybe we
should just add the disables for warnings we don't want that we know
clang or cparser have on by default, to avoid having to worry about -w
discrepancy between gcc and others.

Regarding -Wdangling-else itself, it's still a style rule that's not
followed in musl. The similar -Wmisleading-indentation seems closer to
style we do generally follow and might be appropriate under
--enable-warnings, if it doesn't have any annoying false positives.

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] fix warning dangling-else
  2019-07-23  3:38     ` Rich Felker
@ 2019-07-23  4:06       ` Fangrui Song
  2019-07-23  4:25         ` Rich Felker
  0 siblings, 1 reply; 11+ messages in thread
From: Fangrui Song @ 2019-07-23  4:06 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

On 2019-07-22, Rich Felker wrote:
>On Tue, Jul 23, 2019 at 02:31:24AM +0000, Fangrui Song wrote:
>> With the attached patch, gcc has just some warnings in src/ctype/towctrans.c
>>
>> [-Wdangling-else]
>>  supposedly it will be address soon: "In the case of patch 1 here,
>>  there's actually a pending replacement implementation for the whole
>>  file."
>>
>> clang has a few more:
>>
>> % grep -o '\[-.*\]' /tmp/clang.log | sort | uniq -c
>> 4 [-Wdangling-else]
>> 10 [-Wignored-attributes]
>>     they are all in the form of `weak_alias(statfs, statfs64)`.
>>     these warnings will go away when the lfs64 things are fixed.
>> 18 [-Wunknown-pragmas]
>>     src/math/fmal.c:167:15: warning: pragma STDC FENV_ACCESS ON is not supported, ignoring pragma [-Wunknown-pragmas]
>>             #pragma STDC FENV_ACCESS ON
>>     There is a long-standing bug https://bugs.llvm.org/show_bug.cgi?id=8100
>>     "[llvm-dev] [cfe-dev] Why is #pragma STDC FENV_ACCESS not supported?" was a 2018 discussion on this topic.
>>
>> [-Wdangling-else] and [-Wignored-attributes] will go away soon.
>
>> From bf24cf2d5717505b5c880d2eb6714789f86a902c Mon Sep 17 00:00:00 2001
>> From: Fangrui Song <i@maskray.me>
>> Date: Tue, 23 Jul 2019 02:02:47 +0000
>> Subject: [PATCH] disable some known-unwanted but enabled-by-default warnings
>>  in clang
>>
>> the known-unwanted -Wstring-plus-int and the warning group -Wparentheses
>> are enabled by default in clang. adjust CFLAGS_AUTO to disable these
>> warnings whether or not --enable-warnings is specified.
>> ---
>>  configure | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index 86801281..7f63a873 100755
>> --- a/configure
>> +++ b/configure
>> @@ -514,7 +514,6 @@ test "$cc_family" = clang && tryflag CFLAGS_AUTO -Qunused-arguments
>>
>>  if test "x$warnings" = xyes ; then
>>  tryflag CFLAGS_AUTO -Wall
>> -tryflag CFLAGS_AUTO -Wno-parentheses
>>  tryflag CFLAGS_AUTO -Wno-uninitialized
>>  tryflag CFLAGS_AUTO -Wno-missing-braces
>>  tryflag CFLAGS_AUTO -Wno-unused-value
>> @@ -522,6 +521,9 @@ tryflag CFLAGS_AUTO -Wno-unused-but-set-variable
>>  tryflag CFLAGS_AUTO -Wno-unknown-pragmas
>>  tryflag CFLAGS_AUTO -Wno-pointer-to-int-cast
>>  fi
>> +tryflag CFLAGS_AUTO -Wno-string-plus-int
>> +tryflag CFLAGS_AUTO -Wno-parentheses
>> +tryflag CFLAGS_AUTO -Wdangling-else
>
>Why is the patch adding a test to *enable* a warning outside of the
>--enable-warnings case? The -Wno's here make sense -- maybe we
>should just add the disables for warnings we don't want that we know
>clang or cparser have on by default, to avoid having to worry about -w
>discrepancy between gcc and others.
>
>Regarding -Wdangling-else itself, it's still a style rule that's not
>followed in musl. The similar -Wmisleading-indentation seems closer to
>style we do generally follow and might be appropriate under
>--enable-warnings, if it doesn't have any annoying false positives.

The annoying group -Wparentheses is enabled by default in clang.
-Wdangling-else is within the group. I incorrectly thought it is
desired (in my own projects I don't like these warnings, but oftentimes I
just submit to the default warning rule..)

If -Wmisleading-indentation (not supported by clang yet) captured the
following case, I would agree it is strictly better than
-Wdangling-else:

  if (...)
    if (...)
      ;
  else
    ;

--- c/configure
+++ w/configure
@@ -517 +516,0 @@ tryflag CFLAGS_AUTO -Wall
-tryflag CFLAGS_AUTO -Wno-parentheses
@@ -524,0 +524,2 @@ fi
+tryflag CFLAGS_AUTO -Wno-string-plus-int
+tryflag CFLAGS_AUTO -Wno-parentheses

Some clang packages may ship the tool "diagtool", diagtool tree gives a
hierarchy of warnings/warning groups. The green ones mark "enabled-by-default warnings.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] fix warning dangling-else
  2019-07-23  4:06       ` Fangrui Song
@ 2019-07-23  4:25         ` Rich Felker
  0 siblings, 0 replies; 11+ messages in thread
From: Rich Felker @ 2019-07-23  4:25 UTC (permalink / raw)
  To: musl

On Tue, Jul 23, 2019 at 04:06:31AM +0000, Fangrui Song wrote:
> On 2019-07-22, Rich Felker wrote:
> >On Tue, Jul 23, 2019 at 02:31:24AM +0000, Fangrui Song wrote:
> >>With the attached patch, gcc has just some warnings in src/ctype/towctrans.c
> >>
> >>[-Wdangling-else]
> >> supposedly it will be address soon: "In the case of patch 1 here,
> >> there's actually a pending replacement implementation for the whole
> >> file."
> >>
> >>clang has a few more:
> >>
> >>% grep -o '\[-.*\]' /tmp/clang.log | sort | uniq -c
> >>4 [-Wdangling-else]
> >>10 [-Wignored-attributes]
> >>    they are all in the form of `weak_alias(statfs, statfs64)`.
> >>    these warnings will go away when the lfs64 things are fixed.
> >>18 [-Wunknown-pragmas]
> >>    src/math/fmal.c:167:15: warning: pragma STDC FENV_ACCESS ON is not supported, ignoring pragma [-Wunknown-pragmas]
> >>            #pragma STDC FENV_ACCESS ON
> >>    There is a long-standing bug https://bugs.llvm.org/show_bug.cgi?id=8100
> >>    "[llvm-dev] [cfe-dev] Why is #pragma STDC FENV_ACCESS not supported?" was a 2018 discussion on this topic.
> >>
> >>[-Wdangling-else] and [-Wignored-attributes] will go away soon.
> >
> >>From bf24cf2d5717505b5c880d2eb6714789f86a902c Mon Sep 17 00:00:00 2001
> >>From: Fangrui Song <i@maskray.me>
> >>Date: Tue, 23 Jul 2019 02:02:47 +0000
> >>Subject: [PATCH] disable some known-unwanted but enabled-by-default warnings
> >> in clang
> >>
> >>the known-unwanted -Wstring-plus-int and the warning group -Wparentheses
> >>are enabled by default in clang. adjust CFLAGS_AUTO to disable these
> >>warnings whether or not --enable-warnings is specified.
> >>---
> >> configure | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/configure b/configure
> >>index 86801281..7f63a873 100755
> >>--- a/configure
> >>+++ b/configure
> >>@@ -514,7 +514,6 @@ test "$cc_family" = clang && tryflag CFLAGS_AUTO -Qunused-arguments
> >>
> >> if test "x$warnings" = xyes ; then
> >> tryflag CFLAGS_AUTO -Wall
> >>-tryflag CFLAGS_AUTO -Wno-parentheses
> >> tryflag CFLAGS_AUTO -Wno-uninitialized
> >> tryflag CFLAGS_AUTO -Wno-missing-braces
> >> tryflag CFLAGS_AUTO -Wno-unused-value
> >>@@ -522,6 +521,9 @@ tryflag CFLAGS_AUTO -Wno-unused-but-set-variable
> >> tryflag CFLAGS_AUTO -Wno-unknown-pragmas
> >> tryflag CFLAGS_AUTO -Wno-pointer-to-int-cast
> >> fi
> >>+tryflag CFLAGS_AUTO -Wno-string-plus-int
> >>+tryflag CFLAGS_AUTO -Wno-parentheses
> >>+tryflag CFLAGS_AUTO -Wdangling-else
> >
> >Why is the patch adding a test to *enable* a warning outside of the
> >--enable-warnings case? The -Wno's here make sense -- maybe we
> >should just add the disables for warnings we don't want that we know
> >clang or cparser have on by default, to avoid having to worry about -w
> >discrepancy between gcc and others.
> >
> >Regarding -Wdangling-else itself, it's still a style rule that's not
> >followed in musl. The similar -Wmisleading-indentation seems closer to
> >style we do generally follow and might be appropriate under
> >--enable-warnings, if it doesn't have any annoying false positives.
> 
> The annoying group -Wparentheses is enabled by default in clang.
> -Wdangling-else is within the group. I incorrectly thought it is
> desired (in my own projects I don't like these warnings, but oftentimes I
> just submit to the default warning rule..)

I see, I missed that you were "undoing" part of the -Wno-parentheses.
But I still would just leave it out; it's not really wanted.

> If -Wmisleading-indentation (not supported by clang yet) captured the
> following case, I would agree it is strictly better than
> -Wdangling-else:
> 
>  if (...)
>    if (...)
>      ;
>  else
>    ;

I think it does but I'm not sure. I'm mildly worried about unfixable
false positives in the case of #if tho -- things like:

#if ...
	if (foo)
		...;
	else
#endif
	...;

Which are going to be needed a lot to deal with the kernel mess of
omitting random sets of syscalls on each arch, in implementing the
right fallback chains for time64 stuff...

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-07-23  4:25 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-22 18:07 [PATCH 1/5] fix warning dangling-else Issam Maghni
2019-07-22 18:07 ` [PATCH 2/5] fix warning logical-op-parentheses Issam Maghni
2019-07-22 18:07 ` [PATCH 3/5] fix warning bitwise-op-parentheses Issam Maghni
2019-07-22 18:07 ` [PATCH 4/5] fix warning shift-op-parentheses Issam Maghni
2019-07-22 18:07 ` [PATCH 5/5] fix warning parentheses Issam Maghni
2019-07-22 18:50 ` [PATCH 1/5] fix warning dangling-else A. Wilcox
2019-07-22 20:50 ` Rich Felker
2019-07-23  2:31   ` Fangrui Song
2019-07-23  3:38     ` Rich Felker
2019-07-23  4:06       ` Fangrui Song
2019-07-23  4:25         ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).