[1/5] fix warning dangling-else

Submitted by Fangrui Song on July 23, 2019, 4:06 a.m.

Details

Message ID 20190723040631.a7ug6glq62hkfpzz@gmail.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Fangrui Song July 23, 2019, 4:06 a.m.
On 2019-07-22, Rich Felker wrote:
>On Tue, Jul 23, 2019 at 02:31:24AM +0000, Fangrui Song wrote:
>> 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.
>
>> From bf24cf2d5717505b5c880d2eb6714789f86a902c Mon Sep 17 00:00:00 2001
>> From: Fangrui Song <i@maskray.me>
>> 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
>
>Why is the patch adding a test to *enable* a warning outside of the
>--enable-warnings case? The -Wno's here make sense -- maybe we
>should just add the disables for warnings we don't want that we know
>clang or cparser have on by default, to avoid having to worry about -w
>discrepancy between gcc and others.
>
>Regarding -Wdangling-else itself, it's still a style rule that's not
>followed in musl. The similar -Wmisleading-indentation seems closer to
>style we do generally follow and might be appropriate under
>--enable-warnings, if it doesn't have any annoying false positives.

The annoying group -Wparentheses is enabled by default in clang.
-Wdangling-else is within the group. I incorrectly thought it is
desired (in my own projects I don't like these warnings, but oftentimes I
just submit to the default warning rule..)

If -Wmisleading-indentation (not supported by clang yet) captured the
following case, I would agree it is strictly better than
-Wdangling-else:

  if (...)
    if (...)
      ;
  else
    ;


Some clang packages may ship the tool "diagtool", diagtool tree gives a
hierarchy of warnings/warning groups. The green ones mark "enabled-by-default warnings.

Patch hide | download patch | download mbox

--- c/configure
+++ w/configure
@@ -517 +516,0 @@  tryflag CFLAGS_AUTO -Wall
-tryflag CFLAGS_AUTO -Wno-parentheses
@@ -524,0 +524,2 @@  fi
+tryflag CFLAGS_AUTO -Wno-string-plus-int
+tryflag CFLAGS_AUTO -Wno-parentheses

Comments

Rich Felker July 23, 2019, 4:25 a.m.
On Tue, Jul 23, 2019 at 04:06:31AM +0000, Fangrui Song wrote:
> On 2019-07-22, Rich Felker wrote:
> >On Tue, Jul 23, 2019 at 02:31:24AM +0000, Fangrui Song wrote:
> >>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.
> >
> >>From bf24cf2d5717505b5c880d2eb6714789f86a902c Mon Sep 17 00:00:00 2001
> >>From: Fangrui Song <i@maskray.me>
> >>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
> >
> >Why is the patch adding a test to *enable* a warning outside of the
> >--enable-warnings case? The -Wno's here make sense -- maybe we
> >should just add the disables for warnings we don't want that we know
> >clang or cparser have on by default, to avoid having to worry about -w
> >discrepancy between gcc and others.
> >
> >Regarding -Wdangling-else itself, it's still a style rule that's not
> >followed in musl. The similar -Wmisleading-indentation seems closer to
> >style we do generally follow and might be appropriate under
> >--enable-warnings, if it doesn't have any annoying false positives.
> 
> The annoying group -Wparentheses is enabled by default in clang.
> -Wdangling-else is within the group. I incorrectly thought it is
> desired (in my own projects I don't like these warnings, but oftentimes I
> just submit to the default warning rule..)

I see, I missed that you were "undoing" part of the -Wno-parentheses.
But I still would just leave it out; it's not really wanted.

> If -Wmisleading-indentation (not supported by clang yet) captured the
> following case, I would agree it is strictly better than
> -Wdangling-else:
> 
>  if (...)
>    if (...)
>      ;
>  else
>    ;

I think it does but I'm not sure. I'm mildly worried about unfixable
false positives in the case of #if tho -- things like:

#if ...
	if (foo)
		...;
	else
#endif
	...;

Which are going to be needed a lot to deal with the kernel mess of
omitting random sets of syscalls on each arch, in implementing the
right fallback chains for time64 stuff...

Rich