[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 23 Jul 2019 02:31:24 +0000
From: Fangrui Song <i@...kray.me>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH 1/5] fix warning dangling-else
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@...cati.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.
View attachment "musl.patch" of type "text/x-diff" (1345 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
