From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14427 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Fangrui Song Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH 1/5] fix warning dangling-else Date: Tue, 23 Jul 2019 02:31:24 +0000 Message-ID: <20190723023124.zvfztree4fs7fonb@gmail.com> References: <20190722180740.16951-1-me@concati.me> <20190722205009.GA11895@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="n2wj2mfctlok3j4s" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="57449"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: NeoMutt/20180223-112-0c5bf3 Cc: musl@lists.openwall.com To: Rich Felker Original-X-From: musl-return-14443-gllmg-musl=m.gmane.org@lists.openwall.com Tue Jul 23 04:31:45 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1hpkae-000Eq7-Mw for gllmg-musl@m.gmane.org; Tue, 23 Jul 2019 04:31:44 +0200 Original-Received: (qmail 13696 invoked by uid 550); 23 Jul 2019 02:31:41 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 13623 invoked from network); 23 Jul 2019 02:31:40 -0000 X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=f362DvXKS16YOGCCnSyglmhxBZ/05vS1P0NB1ic9qe8=; b=dspRGj5eyFDZoTjffQMiZzhBKEOmtMgMNntP7cCDbmCFg1usZxseRcVuMh5zBEfccY Q0+EgDYI/r6Dg0z/sem4muvktSaqjPTTMTQecMk2bPkai/pD6iOG0b4tjU+6Can0+c0y Mb8G42UeKmM+j9DQr1Q+7SYAw2pp/+9m7BxD5f9NWp5A/y9FkRyXz4D1ams8DJbBtPs3 jJVGZftX51pNGVmvjWSV8GaDEcWm+BZsIErQVYKlUfClCikwqVjawjWdf3Xf+d9GpKCt 5DcEkRFfOK+njpAiJxdMQ98ZhKXIzaVOzr1vT17cQ8kDoYpKCXafcqZlF4XwLbIKK6qW dkqw== X-Gm-Message-State: APjAAAW6LenOn+JlIYCxMUrnZZVMvFGXsD+CHjHhVLedeBH3ThPyZvkH MycTWEX10LfLJxxV3qIMtsk= X-Google-Smtp-Source: APXvYqzJxDtXvPo/TFhZzfHCBQjRVTRGnEybNWo9NfT+fU3G9TVqc8jyZ+ELaKo80YMJx4dF4c831Q== X-Received: by 2002:a63:6086:: with SMTP id u128mr10968346pgb.158.1563849087928; Mon, 22 Jul 2019 19:31:27 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20190722205009.GA11895@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:14427 Archived-At: --n2wj2mfctlok3j4s Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline 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. --n2wj2mfctlok3j4s Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="musl.patch" >From bf24cf2d5717505b5c880d2eb6714789f86a902c Mon Sep 17 00:00:00 2001 From: Fangrui Song 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 --n2wj2mfctlok3j4s--