Re: Implement -Wimplicit-fallthrough (version 7)

[Marek Polacek <polacek at redhat dot com>]

On Tue, Aug 30, 2016 at 07:38:18AM -0400, Eric Gallager wrote:
> On 8/30/16, Marek Polacek <polacek@redhat.com> wrote:
> > On Mon, Aug 29, 2016 at 09:32:04AM -0400, Eric Gallager wrote:
> >> I tried v6 on my binutils-gdb fork, and it printed A LOT of
> >> warnings...
> >
> > BTW, why is that so?  Does binutils-gdb not use various FALLTHRU comments?
> >
> > 	Marek
> >
> 
> 
> There are a lot of comments mentioning fallthroughs, but they mostly
> need to be rewritten to be recognized properly. There's just so many
> different ways of writing them. For example, one I've seen a lot
> writes it as "Drop through" instead of "Fall through" or something.
> Other examples of comments mentioning fallthroughs:
> 
> /* else fall through */
> /* else fallthrough to: */
> /* FALL THRU into number case.  */
> /* ObjC NSString constant: fall through and parse like STRING. */
> /* Fall through, pretend it is global.  */
> /* Fall through into normal member function.  */
> /* fall in for static symbols that do NOT start with '.' */
> /* >>> !! else fall through !! <<< */
> /* ... fall through for unsigned ints ... */
> /* fall thru to manual case */
 
Thanks, this is interesting.  I wonder if we should also recognize
"else fall through"; I've seen that in our codebase a few times.

But why would anyone write something as ugly as ">>> !! else fall through !! <<<"
is beyond me.

> Also, at second glance, it's actually not quite as many warnings as I
> thought it was at first, it mostly just looked that way since each
> -Wimplicit-fallthrough warning takes up 12 lines due to the
> double-fixits. FWIW, Aldy's -Walloca-larger-than patch actually was
> noisier in the GDB portion at least. (The Binutils portion was already
> clean on its alloca usage since it already used the -Wstack-usage
> warning.)

That's nice to hear.  The latest version of the patch doesn't have the fix-it
hints, it just says in an inform note where a statement falls through to,
so it's going to be less verbose.

> Oh, and one other thing I discovered:
> __attribute__((fallthrough)) triggers -Wdeclaration-after-statement:
> 
> In file included from ./defs.h:124:0,
>                  from ./cli/cli-setshow.c:20:
> ./cli/cli-setshow.c: In function ‘do_setshow_command’:
> ./../include/ansidecl.h:505:33: warning: ISO C90 forbids mixed
> declarations and code [-Wdeclaration-after-statement]
>  #  define ATTRIBUTE_FALLTHROUGH __attribute__((fallthrough))
>                                  ^
> ./cli/cli-setshow.c:359:4: note: in expansion of macro ‘ATTRIBUTE_FALLTHROUGH’
>     ATTRIBUTE_FALLTHROUGH;
>     ^~~~~~~~~~~~~~~~~~~~~
> 
> Which doesn't really make sense to me.

Yes, I think this is a bug.  I'm pondering how to fix this.

Thanks a lot Eric!

	Marek