On Wed, Oct 12, 2016 at 11:52:02AM +0200, Bernd Schmidt wrote:
> On 10/12/2016 11:31 AM, Markus Trippelsdorf wrote:
> >On 2016.10.12 at 00:34 +0200, Bernd Schmidt wrote:
> >>It's a discussion we should have, but I agree it should be done
> >>incrementally. I would argue for =1 as the default.
> >
> >Here are some numbers for an allmodconfig Linux kernel on pcc64le:
> >
> >-Wimplicit-fallthrough=1 : 951 warnings
> >-Wimplicit-fallthrough=2 : 1087 warnings
> >-Wimplicit-fallthrough=3 : 1209 warnings
> >
> >I randomly looked at the differences and almost all additional
> >-Wimplicit-fallthrough=2 warnings are bogus (~5% are genuine).
> >And _all_ additional -Wimplicit-fallthrough=3 warnings appear
> >to be bogus.
>
> And that's for a codebase that was written in English to begin with. Would
> you mind posting one or two examples if you saw interesting ones, for
> reference?
>
> This result suggests that we should probably collapse levels 3-5 into a
> single strict one that doesn't try to be clever, and definitely make at most
> level 1 the default.
What do you mean at most? level 0 is the warning disabled, that is the
default except for -Wextra. The difference between =1 and =2 is very small
amount of warnings, one will need to annotate or add break; to those 951
spots anyway to make it -Wextra clean (those that don't have any kind of
comment at all), so just handling the additional 136 ones as well is not
that big deal. It would be interesting to see those 136 comments though,
whether anything in them is about the intentional fall through or if they
are just unrelated.
Collapsing 3-5 levels is a bad idea, various projects will want to only
rely on attributes, especially if other compilers only support attributes
and nothing else, which is why there is level 5. Levels 4 and 3 give choice
on how much free form the comments are.
> Another thing, was it ever resolved what this warning does to tools like
> ccache which like to operate on preprocessed files?
We already have 2 real-world PRs about cases like:
case 2:
bar ();
/* FALLTHRU */
#undef X
case 3:
#ifdef Y
bar ();
/* FALLTHRU */
#endif
case 4:
bar ();
not being handled, which are extremely difficult to handle the current way
in libcpp, there can be many tokens in between the fallthrough_comment_p
comments
and the case/default/label tokens. It should work fine with -C or -CC.
So I'm wondering if a better approach wouldn't be that for
-Wimplicit-fallthrough={1,2,3,4,5} we'd let the fallthrough_comment_p
comments get through (perhaps normalized if not -C/-CC) as CPP_COMMENT with
the PREV_FALLTHROUGH flag and perhaps also another one that would indicate
it is really whitespace for other purposes. The -C/-CC description talks
about significant differences though, e.g.
/*FALLTHROUGH*/#define A 1
A
is preprocessed differently with -C/-CC and without, so if we want to go
that way, we'd also need to special case the non-C/CC added CPP_COMMENT.
I have no idea what else it would affect though :(, I'm worried about token
pasting and lots of other cases.
If that is resolved, we could just emit the normalized /*FALLTHROUGH*/
comments if {-Wextra,-W,-Wimplicit-fallthrough{,=1,=2,=3,=4}} into -E output
too and just document that we do that.
Of course, for ccache I keep suggesting that they use -fdirectives-only
preprocessing instead, because anything else breaks miserably tons of other
stuff we have added into GCC over the last decade (-Wmisleading-indentation,
-Wsystem-headers vs. macro contexts, etc.), but the maintainers disagree.
Jakub
