Re: Implement -Wimplicit-fallthrough (version 7)

[Eric Gallager <egall at gwmail dot gwu dot edu>]

On 8/29/16, David Malcolm <dmalcolm@redhat.com> wrote:
> On Mon, 2016-08-29 at 09:32 -0400, Eric Gallager wrote:
>> On 8/29/16, Marek Polacek <polacek@redhat.com> wrote:
>> > Tobias tried my latest version and reported some ICEs.  They should
>> > all be
>> > fixed in this version (the only change since version 6 is the
>> > cp/pt.c
>> > hunk).
>> >
>> > At this point I'd like to ask Jason and Joseph to review the C/C++
>> > parts
>> > and someone to review the ME parts so that I can finally wrap this
>> > thing
>> > up.  This warning found a couple of bugs in our codebase and I
>> > suspect it
>> > will find some in other codebases, too.
>> >
>> > Does anyone have any concerns that I haven't addressed yet?
>> >
>> > Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for
>> > trunk?
>> >
>>
>>
>> I tried v6 on my binutils-gdb fork, and it printed A LOT of
>> warnings... After this patch goes in, the main question I'd have
>> would
>> be about the carets: it seems like it would make more sense for me
>> for
>> the location of the warning to be swapped with that of the fixit
>> hint(s)? With the fixit pointing to the case label before the
>> statement that's warned about, it makes it look like it's suggesting
>> to put the fallthrough attribute or the break before the rest of the
>> content of the case, which, with the break, could lead to dead code.
>> I'd think it'd make more sense to point to after the body of the case
>> statement instead...
>
> Interesting.  Please can you post an example of the output that you're
> referring to?
>
> I'm working on improvements to how we print fix-its, so I'm wondering
> if this is an issue with the fix-it data, or with the presentation of
> it.
>
> Thanks
> Dave
>

Sure, an example of the warning looks like this for me:

./cli/cli-setshow.c: In function ‘do_setshow_command’:
./cli/cli-setshow.c:353:7: warning: this statement may fall through
[-Wimplicit-fallthrough]
    if (*(unsigned int *)c->var == UINT_MAX)
       ^
./cli/cli-setshow.c:359:2: note: insert ‘__attribute__
((fallthrough));’ to silence this warning
  case var_zinteger:
  ^~~~
  __attribute__ ((fallthrough));
./cli/cli-setshow.c:359:2: note: insert ‘break;’ to avoid fall-through
  case var_zinteger:
  ^~~~
  break;

Eric