From: Rich Felker <dalias@libc.org>
To: musl@lists.openwall.com
Subject: Re: [PATCH 1/5] fix warning dangling-else
Date: Tue, 23 Jul 2019 00:25:17 -0400 [thread overview]
Message-ID: <20190723042517.GA16460@brightrain.aerifal.cx> (raw)
In-Reply-To: <20190723040631.a7ug6glq62hkfpzz@gmail.com>
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
prev parent reply other threads:[~2019-07-23 4:25 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-22 18:07 Issam Maghni
2019-07-22 18:07 ` [PATCH 2/5] fix warning logical-op-parentheses Issam Maghni
2019-07-22 18:07 ` [PATCH 3/5] fix warning bitwise-op-parentheses Issam Maghni
2019-07-22 18:07 ` [PATCH 4/5] fix warning shift-op-parentheses Issam Maghni
2019-07-22 18:07 ` [PATCH 5/5] fix warning parentheses Issam Maghni
2019-07-22 18:50 ` [PATCH 1/5] fix warning dangling-else A. Wilcox
2019-07-22 20:50 ` Rich Felker
2019-07-23 2:31 ` Fangrui Song
2019-07-23 3:38 ` Rich Felker
2019-07-23 4:06 ` Fangrui Song
2019-07-23 4:25 ` Rich Felker [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190723042517.GA16460@brightrain.aerifal.cx \
--to=dalias@libc.org \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).