From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14425 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH 1/5] fix warning dangling-else Date: Mon, 22 Jul 2019 16:50:09 -0400 Message-ID: <20190722205009.GA11895@brightrain.aerifal.cx> References: <20190722180740.16951-1-me@concati.me> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="26055"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-14441-gllmg-musl=m.gmane.org@lists.openwall.com Mon Jul 22 22:50:26 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 1hpfGL-0006gb-Jw for gllmg-musl@m.gmane.org; Mon, 22 Jul 2019 22:50:25 +0200 Original-Received: (qmail 24054 invoked by uid 550); 22 Jul 2019 20:50:23 -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 24009 invoked from network); 22 Jul 2019 20:50:22 -0000 Content-Disposition: inline In-Reply-To: <20190722180740.16951-1-me@concati.me> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:14425 Archived-At: 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