Implement -Wimplicit-fallthrough (take 3)

[Marek Polacek]

On Thu, Aug 18, 2016 at 01:07:26PM -0400, David Malcolm wrote:
> On Thu, 2016-08-18 at 15:50 +0200, Marek Polacek wrote:
> > Now that all various switch fallthrough bugfixes and adjustments were
> > committed, and this patch has shrunk considerably, I'm presenting the
> > latest
> > version.  The changes from the last version are not huge; we don't
> > warn for a
> > fall through to a user-defined label anymore, and I made some tiny
> > changes
> > regarding parsing attributes in the C FE, as requested by Joseph.
> > 
> > This patch is accompanied by another patch that merely adds various
> > gcc_fallthroughs, in places where a FALLTHRU comment wouldn't work.
> > 
> > It's been tested on powerpc64le-unknown-linux-gnu, aarch64-linux-gnu,
> > and x86_64-redhat-linux.
> > 
> > 2016-08-18  Marek Polacek  <polacek@redhat.com>
> > 	    Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR c/7652
> 
> [...]
>  
> > diff --git gcc/gcc/gimplify.c gcc/gcc/gimplify.c
> > index 1e43dbb..1925263 100644
> > --- gcc/gcc/gimplify.c
> > +++ gcc/gcc/gimplify.c
> 
> [...]
> 
> > +	    if (warned_p)
> > +	      {
> > +		rich_location richloc (line_table, gimple_location
> > (next));
> > +		richloc.add_fixit_insert (gimple_location (next),
> > +					  "insert '__attribute__ "
> > +					  "((fallthrough));' to
> > silence "
> > +					  "this warning");
> > +		richloc.add_fixit_insert (gimple_location (next),
> > +					  "insert 'break;' to avoid
> > "
> > +					  "fall-through");
> > +		inform_at_rich_loc (&richloc, "here");
> > +	      }
> 
> This isn't quite the way that fix-its are meant to be used, in my mind,
> at least: the insertion text is supposed to be something that could be
> literally inserted into the code (e.g. by an IDE) i.e. it should be a
> code fragment, rather than a message to the user.
> 
> Here's an idea of how the above could look:
> 
>   if (warned_p)
>     {
>       /* Suggestion one: add "__attribute__ ((fallthrough));".  */
>       rich_location richloc_attr (line_table, gimple_location (next));
>       richloc_attr.add_fixit_insert (gimple_location (next),
>                                      "__attribute__ ((fallthrough));");
>       inform_at_rich_loc (&richloc_attr, "insert %qs to silence this warning",
>                           "__attribute__ ((fallthrough));")
> 
>       /* Suggestion two: add "break;".  */
>       rich_location richloc_break (line_table, gimple_location (next));
>       richloc_break.add_fixit_insert (gimple_location (next),
>                                       "break;");
>       inform_at_rich_loc (&richloc_break, "insert %qs to avoid fall-through",
>                           "break;");
>     }
> 
> 
> (and using %qs for quoting the language elements in the messages
> themselves).
 
Thanks for the clarification, I made it so and the output looks *much* better.

> There doesn't seem to be any test coverage in the patch for the fix-it
> hints.
> 
> The easiest way to do this is to create a test case with:
> 
> /* { dg-options "-fdiagnostics-show-caret" } */
> 
> and then add:
> 
>  /* { dg-begin-multiline-output "" }
> QUOTE OF EXPECTED CARET+FIXIT OUTPUT, omitting any trailing deja-gnu
> directives
>      { dg-end-multiline-output "" } */
> 
> so that we can directly verify that the results look sane.
 
Ok, I added a testcase testing multiline comments.

> BTW, is there some way to break up warn_implicit_fallthrough_r, maybe
> moving the GIMPLE_LABEL handling to a subroutine? (and perhaps the
> suggestion-handling above could live in its own subroutine, etc).

Let me try to split it up a bit.

Thanks,

	Marek