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

[Rich Felker <dalias@libc.org>]

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