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