On Wed, Oct 12, 2016 at 12:08:40PM +0200, Markus Trippelsdorf wrote:
> On 2016.10.12 at 11:52 +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?
> 
> Actually looking more closely it appears that all of the 136 additional
> warnings for level 2 are bogus, too. Here is an example:
> 
>          switch (class) {
>          case ATA_DEV_SEMB:
>                  class = ATA_DEV_ATA;    /* some hard drives report SEMB sig 
> */
>          case ATA_DEV_ATA:
>          case ATA_DEV_ZAC:
>                  tf.command = ATA_CMD_ID_ATA;
>                  break;

This is a clear argument for -Wimplicit-fallthrough default to =2 or higher.
The comment is completely random, doesn't express in any way the intent to
fall through, and if the comment appeared one line earlier (just slightly
different coding style), we'd warn.  I don't see a difference between why
we shouldn't warn by default (still talking about -Wextra) for the above
and should for:
        switch (class) {
        case ATA_DEV_SEMB:
                class = ATA_DEV_ATA;
        case ATA_DEV_ATA:
        case ATA_DEV_ZAC:
                tf.command = ATA_CMD_ID_ATA;
                break;
In neither case the warning is bogus, it isn't even clear without additional
code analysis whether it is a code bug or intentational fallthrough not
marked that way.  The reason for adding =1 mode has been to support projects
that can't be bothered to express their intent in any standardized way and
want to express it free form in arbitrary languages, using various wordings
etc.  At the expense of missing cases like the above, where they actually
don't express the intent in any way.

        Jakub

Reply via email to