Re: [PATCH 1/5] fix warning dangling-else

[Fangrui Song <i@maskray.me>]

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