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 <pola...@redhat.com>
> > Jakub Jelinek <ja...@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
