This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Implement -Wimplicit-fallthrough (take 3)


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).

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.

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).

[...]

Hope this is constructive
Dave


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]