[PATCH] Fix-it hints for -Wimplicit-fallthrough

[PATCH] Fix-it hints for -Wimplicit-fallthrough

[David Malcolm]

As of r247522, fix-it-hints can suggest the insertion of new lines.

This patch updates -Wimplicit-fallthrough to provide suggestions
with fix-it hints, showing the user where to insert "break;" or
fallthrough attributes.

For example:

 test.c: In function 'set_x':
 test.c:15:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
        x = a;
        ~~^~~
 test.c:22:5: note: here
      case 'b':
      ^~~~
 test.c:22:5: note: insert '__attribute__ ((fallthrough));' to silence this warning
 +        __attribute__ ((fallthrough));
      case 'b':
      ^~~~
 test.c:22:5: note: insert 'break;' to avoid fall-through
 +        break;
      case 'b':
      ^~~~

The idea is that if an IDE supports -fdiagnostics-parseable-fixits, the
user can fix these issues by clicking on them.

It's loosely based on part of Marek's patch:
  https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01948.html
but with extra logic for locating a suitable place to insert the
fix-it hint, so that, if possible, it is on a line by itself before
the "case", indented the same as the prior statement.  If that's not
possible, no fix-it hint is emitted.

In Marek's patch he wrote:
  /* For C++17, we'd recommend [[fallthrough]];, but we're not
     there yet.  For C++11, recommend [[gnu::fallthrough]];.  */
"[[fallthrough]]" appears to work, but it appears that lang_hooks.name
doesn't expose C++17 yet, so the patch recommends [[gnu::fallthrough]
for C++17.


Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

OK for trunk?

gcc/ChangeLog:
	PR c/7652
	* diagnostic.c (diagnostic_report_diagnostic): Only add fixits
	to the edit_context if they can be auto-applied.
	* gimplify.c (add_newline_fixit_with_indentation): New function.
	(warn_implicit_fallthrough_r): Add suggestions on how to fix
	-Wimplicit-fallthrough.

gcc/testsuite/ChangeLog:
	PR c/7652
	* g++.dg/Wimplicit-fallthrough-fixit-c++11.C: New test case.
	* g++.dg/Wimplicit-fallthrough-fixit-c++98.C: New test case.
	* gcc.dg/Wimplicit-fallthrough-fixit.c: New test case.

libcpp/ChangeLog:
	PR c/7652
	* include/line-map.h
	(rich_location::fixits_cannot_be_auto_applied): New method.
	(rich_location::fixits_can_be_auto_applied_p): New accessor.
	(rich_location::m_fixits_cannot_be_auto_applied): New field.
	* line-map.c (rich_location::rich_location): Initialize new field.
---
 gcc/diagnostic.c                                   |  3 +-
 gcc/gimplify.c                                     | 90 +++++++++++++++++++++-
 .../g++.dg/Wimplicit-fallthrough-fixit-c++11.C     | 43 +++++++++++
 .../g++.dg/Wimplicit-fallthrough-fixit-c++98.C     | 43 +++++++++++
 gcc/testsuite/gcc.dg/Wimplicit-fallthrough-fixit.c | 73 ++++++++++++++++++
 libcpp/include/line-map.h                          | 22 ++++++
 libcpp/line-map.c                                  |  3 +-
 7 files changed, 274 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++11.C
 create mode 100644 gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++98.C
 create mode 100644 gcc/testsuite/gcc.dg/Wimplicit-fallthrough-fixit.c

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index dc81755..40509f1 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -942,7 +942,8 @@ diagnostic_report_diagnostic (diagnostic_context *context,
   diagnostic->x_data = NULL;
 
   if (context->edit_context_ptr)
-    context->edit_context_ptr->add_fixits (diagnostic->richloc);
+    if (diagnostic->richloc->fixits_can_be_auto_applied_p ())
+      context->edit_context_ptr->add_fixits (diagnostic->richloc);
 
   context->lock--;
 
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index fd27eb1..19dd6dc 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2038,6 +2038,60 @@ should_warn_for_implicit_fallthrough (gimple_stmt_iterator *gsi_p, tree label)
   return true;
 }
 
+/* Attempt to add a fix-it hint to RICHLOC, suggesting the insertion
+   of a new line containing LINE_CONTENT (which does not need
+   a '\n' character).
+   The new line is to be inserted immediately before the
+   line containing NEXT, using the location of PREV to determine
+   indentation.
+   The new line will only be inserted if there is nothing on
+   NEXT's line before NEXT, and we can determine the indentation
+   of PREV.  */
+
+static void
+add_newline_fixit_with_indentation (rich_location *richloc,
+				    location_t prev,
+				    location_t next,
+				    const char *line_content)
+{
+  /* Check that the line containing NEXT is blank, up to NEXT.  */
+  int line_width;
+  expanded_location exploc_next = expand_location (next);
+  const char *line
+    = location_get_source_line (exploc_next.file, exploc_next.line,
+				&line_width);
+  if (!line)
+    return;
+  if (line_width < exploc_next.column)
+    return;
+  /* Columns are 1-based.  */
+  for (int column = 1; column < exploc_next.column; ++column)
+    if (!ISSPACE (line[column - 1]))
+      return;
+
+  /* Locate the start of the line containing NEXT.  */
+  const line_map *map = linemap_lookup (line_table, next);
+  if (linemap_macro_expansion_map_p (map))
+    return;
+  const line_map_ordinary *ordmap = linemap_check_ordinary (map);
+  location_t start_of_line
+    = linemap_position_for_line_and_column (line_table, ordmap,
+					    exploc_next.line, 1);
+
+  /* Generate an insertion string, indenting by the amount PREV
+     was indented.  */
+  int prev_column = LOCATION_COLUMN (prev);
+  pretty_printer tmp_pp;
+  pretty_printer *pp = &tmp_pp;
+  /* Columns are 1-based.  */
+  for (int column = 1; column < prev_column; ++column)
+    pp_space (pp);
+  pp_string (pp, line_content);
+  pp_newline (pp);
+
+  richloc->add_fixit_insert_before (start_of_line, pp_formatted_text (pp));
+}
+
 /* Callback for walk_gimple_seq.  */
 
 static tree
@@ -2111,7 +2165,41 @@ warn_implicit_fallthrough_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
 				     OPT_Wimplicit_fallthrough_,
 				     "this statement may fall through");
 	    if (warned_p)
-	      inform (gimple_location (next), "here");
+	      {
+		inform (gimple_location (next), "here");
+
+		/* Suggestion one: add attribute fallthrough.  */
+		rich_location rloc_attr (line_table, gimple_location (next));
+		const char *attr_hint;
+		/* For C++17, we'd recommend [[fallthrough]];, but we're not
+		   there yet.  For C++11, recommend [[gnu::fallthrough]];.  */
+		if (strncmp (lang_hooks.name, "GNU C++14", 9) == 0
+		    || strncmp (lang_hooks.name, "GNU C++11", 9) == 0)
+		  attr_hint = "[[gnu::fallthrough]];";
+		else
+		  /* Otherwise, recommend __attribute__ ((fallthrough));.  */
+		  attr_hint = "__attribute__ ((fallthrough));";
+
+		add_newline_fixit_with_indentation (&rloc_attr,
+						    gimple_location (prev),
+						    gimple_location (next),
+						    attr_hint);
+		rloc_attr.fixits_cannot_be_auto_applied ();
+		inform_at_rich_loc (&rloc_attr,
+				    "insert %qs to silence this warning",
+				    attr_hint);
+
+		/* Suggestion two: add "break;".  */
+		rich_location rloc_break (line_table, gimple_location (next));
+		add_newline_fixit_with_indentation (&rloc_break,
+						    gimple_location (prev),
+						    gimple_location (next),
+						    "break;");
+		rloc_break.fixits_cannot_be_auto_applied ();
+		inform_at_rich_loc (&rloc_break,
+				    "insert %qs to avoid fall-through",
+				    "break;");
+	      }
 
 	    /* Mark this label as processed so as to prevent multiple
 	       warnings in nested switches.  */
diff --git a/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++11.C b/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++11.C
new file mode 100644
index 0000000..afc808e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++11.C
@@ -0,0 +1,43 @@
+/* PR c/7652 */
+/* { dg-do compile } */
+/* { dg-options "-Wimplicit-fallthrough -std=c++11 -fdiagnostics-show-caret -fdiagnostics-generate-patch" } */
+/* We specify -fdiagnostics-generate-patch, but no patch should be
+   generated, since user-intervention is required to choose between
+   two different fix-it hints.  */
+
+int x;
+
+void set_x (char ch, int a, int b)
+{
+  switch (ch)
+    {
+    case 'a':
+      x = a; /* { dg-warning "statement may fall through" } */
+      /* { dg-begin-multiline-output "" }
+       x = a;
+       ~~^~~
+       { dg-end-multiline-output "" } */
+
+    case 'b':    /* { dg-line second_case } */
+      x = b;
+      /* { dg-message "here" "" { target *-*-* } second_case } */
+      /* { dg-begin-multiline-output "" }
+     case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+
+      /* { dg-message "insert '..gnu::fallthrough..;' to silence this warning" "" { target *-*-* } second_case } */
+      /* { dg-begin-multiline-output "" }
++        [[gnu::fallthrough]];
+     case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+
+      /* { dg-message "insert 'break;' to avoid fall-through" "" { target *-*-* } second_case } */
+      /* { dg-begin-multiline-output "" }
++        break;
+     case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+    }
+}
diff --git a/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++98.C b/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++98.C
new file mode 100644
index 0000000..8730b4c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++98.C
@@ -0,0 +1,43 @@
+/* PR c/7652 */
+/* { dg-do compile } */
+/* { dg-options "-Wimplicit-fallthrough -std=c++98 -fdiagnostics-show-caret -fdiagnostics-generate-patch" } */
+/* We specify -fdiagnostics-generate-patch, but no patch should be
+   generated, since user-intervention is required to choose between
+   two different fix-it hints.  */
+
+int x;
+
+void set_x (char ch, int a, int b)
+{
+  switch (ch)
+    {
+    case 'a':
+      x = a; /* { dg-warning "statement may fall through" } */
+      /* { dg-begin-multiline-output "" }
+       x = a;
+       ~~^~~
+       { dg-end-multiline-output "" } */
+
+    case 'b':    /* { dg-line second_case } */
+      x = b;
+      /* { dg-message "here" "" { target *-*-* } second_case } */
+      /* { dg-begin-multiline-output "" }
+     case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+
+      /* { dg-message "insert '__attribute__ \\(\\(fallthrough\\)\\);' to silence this warning" "" { target *-*-* } second_case } */
+      /* { dg-begin-multiline-output "" }
++        __attribute__ ((fallthrough));
+     case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+
+      /* { dg-message "insert 'break;' to avoid fall-through" "" { target *-*-* } second_case } */
+      /* { dg-begin-multiline-output "" }
++        break;
+     case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+    }
+}
diff --git a/gcc/testsuite/gcc.dg/Wimplicit-fallthrough-fixit.c b/gcc/testsuite/gcc.dg/Wimplicit-fallthrough-fixit.c
new file mode 100644
index 0000000..53daca6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wimplicit-fallthrough-fixit.c
@@ -0,0 +1,73 @@
+/* PR c/7652 */
+/* { dg-do compile } */
+/* { dg-options "-Wimplicit-fallthrough -fdiagnostics-show-caret -fdiagnostics-generate-patch" } */
+/* We specify -fdiagnostics-generate-patch, but no patch should be
+   generated, since user-intervention is required to choose between
+   two different fix-it hints.  */
+
+int x;
+
+void set_x (char ch, int a, int b)
+{
+  switch (ch)
+    {
+    case 'a':
+      x = a; /* { dg-warning "statement may fall through" } */
+      /* { dg-begin-multiline-output "" }
+       x = a;
+       ~~^~~
+       { dg-end-multiline-output "" } */
+
+
+    case 'b':    /* { dg-line second_case } */
+      x = b;
+      /* { dg-message "here" "" { target *-*-* } second_case } */
+      /* { dg-begin-multiline-output "" }
+     case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+
+      /* { dg-message "insert '__attribute__ \\(\\(fallthrough\\)\\);' to silence this warning" "" { target *-*-* } second_case } */
+      /* { dg-begin-multiline-output "" }
++        __attribute__ ((fallthrough));
+     case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+
+      /* { dg-message "insert 'break;' to avoid fall-through" "" { target *-*-* } second_case } */
+      /* { dg-begin-multiline-output "" }
++        break;
+     case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+    }
+}
+
+#define FOO
+
+void set_x_2 (char ch, int a, int b)
+{
+  switch (ch)
+    {
+    case 'a':
+      x = a; /* { dg-warning "statement may fall through" } */
+      /* { dg-begin-multiline-output "" }
+       x = a;
+       ~~^~~
+       { dg-end-multiline-output "" } */
+
+FOO case 'b':    /* { dg-line case_2 } */
+      x = b;
+      /* { dg-message "here" "" { target *-*-* } case_2 } */
+      /* { dg-begin-multiline-output "" }
+ FOO case 'b':
+     ^~~~
+	 { dg-end-multiline-output "" } */
+
+      /* This case isn't the first thing on its line, so the suggestions
+	 should not provide fix-it hints.  */
+
+      /* { dg-message "insert '__attribute__ \\(\\(fallthrough\\)\\);' to silence this warning" "" { target *-*-* } case_2 } */
+      /* { dg-message "insert 'break;' to avoid fall-through" "" { target *-*-* } case_2 } */
+    }
+}
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 90d65d7..be3041d 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1663,6 +1663,27 @@ class rich_location
   fixit_hint *get_last_fixit_hint () const;
   bool seen_impossible_fixit_p () const { return m_seen_impossible_fixit; }
 
+  /* Set this if the fix-it hints are not suitable to be
+     automatically applied.
+
+     For example, if you are suggesting more than one
+     mutually exclusive solution to a problem, then
+     it doesn't make sense to apply all of the solutions;
+     manual intervention is required.
+
+     If set, then the fix-it hints in the rich_location will
+     be printed, but will not be added to generated patches,
+     or affect the modified version of the file.  */
+  void fixits_cannot_be_auto_applied ()
+  {
+    m_fixits_cannot_be_auto_applied = true;
+  }
+
+  bool fixits_can_be_auto_applied_p () const
+  {
+    return !m_fixits_cannot_be_auto_applied;
+  }
+
 private:
   bool reject_impossible_fixit (source_location where);
   void stop_supporting_fixits ();
@@ -1686,6 +1707,7 @@ protected:
   semi_embedded_vec <fixit_hint *, MAX_STATIC_FIXIT_HINTS> m_fixit_hints;
 
   bool m_seen_impossible_fixit;
+  bool m_fixits_cannot_be_auto_applied;
 };
 
 /* A fix-it hint: a suggested insertion, replacement, or deletion of text.
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index c4b7cb2..5caaf6b 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -2012,7 +2012,8 @@ rich_location::rich_location (line_maps *set, source_location loc) :
   m_column_override (0),
   m_have_expanded_location (false),
   m_fixit_hints (),
-  m_seen_impossible_fixit (false)
+  m_seen_impossible_fixit (false),
+  m_fixits_cannot_be_auto_applied (false)
 {
   add_range (loc, true);
 }
-- 
1.8.5.3

[PING] Re: [PATCH] Fix-it hints for -Wimplicit-fallthrough

[David Malcolm]

Ping:
  https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00334.html

On Thu, 2017-05-04 at 14:16 -0400, David Malcolm wrote:
> As of r247522, fix-it-hints can suggest the insertion of new lines.
> 
> This patch updates -Wimplicit-fallthrough to provide suggestions
> with fix-it hints, showing the user where to insert "break;" or
> fallthrough attributes.
> 
> For example:
> 
>  test.c: In function 'set_x':
>  test.c:15:9: warning: this statement may fall through [-Wimplicit
> -fallthrough=]
>         x = a;
>         ~~^~~
>  test.c:22:5: note: here
>       case 'b':
>       ^~~~
>  test.c:22:5: note: insert '__attribute__ ((fallthrough));' to
> silence this warning
>  +        __attribute__ ((fallthrough));
>       case 'b':
>       ^~~~
>  test.c:22:5: note: insert 'break;' to avoid fall-through
>  +        break;
>       case 'b':
>       ^~~~
> 
> The idea is that if an IDE supports -fdiagnostics-parseable-fixits,
> the
> user can fix these issues by clicking on them.
> 
> It's loosely based on part of Marek's patch:
>   https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01948.html
> but with extra logic for locating a suitable place to insert the
> fix-it hint, so that, if possible, it is on a line by itself before
> the "case", indented the same as the prior statement.  If that's not
> possible, no fix-it hint is emitted.
> 
> In Marek's patch he wrote:
>   /* For C++17, we'd recommend [[fallthrough]];, but we're not
>      there yet.  For C++11, recommend [[gnu::fallthrough]];.  */
> "[[fallthrough]]" appears to work, but it appears that
> lang_hooks.name
> doesn't expose C++17 yet, so the patch recommends [[gnu::fallthrough]
> for C++17.
> 
> 
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> 
> OK for trunk?
> 
> gcc/ChangeLog:
> 	PR c/7652
> 	* diagnostic.c (diagnostic_report_diagnostic): Only add fixits
> 	to the edit_context if they can be auto-applied.
> 	* gimplify.c (add_newline_fixit_with_indentation): New
> function.
> 	(warn_implicit_fallthrough_r): Add suggestions on how to fix
> 	-Wimplicit-fallthrough.
> 
> gcc/testsuite/ChangeLog:
> 	PR c/7652
> 	* g++.dg/Wimplicit-fallthrough-fixit-c++11.C: New test case.
> 	* g++.dg/Wimplicit-fallthrough-fixit-c++98.C: New test case.
> 	* gcc.dg/Wimplicit-fallthrough-fixit.c: New test case.
> 
> libcpp/ChangeLog:
> 	PR c/7652
> 	* include/line-map.h
> 	(rich_location::fixits_cannot_be_auto_applied): New method.
> 	(rich_location::fixits_can_be_auto_applied_p): New accessor.
> 	(rich_location::m_fixits_cannot_be_auto_applied): New field.
> 	* line-map.c (rich_location::rich_location): Initialize new
> field.
> ---
>  gcc/diagnostic.c                                   |  3 +-
>  gcc/gimplify.c                                     | 90
> +++++++++++++++++++++-
>  .../g++.dg/Wimplicit-fallthrough-fixit-c++11.C     | 43 +++++++++++
>  .../g++.dg/Wimplicit-fallthrough-fixit-c++98.C     | 43 +++++++++++
>  gcc/testsuite/gcc.dg/Wimplicit-fallthrough-fixit.c | 73
> ++++++++++++++++++
>  libcpp/include/line-map.h                          | 22 ++++++
>  libcpp/line-map.c                                  |  3 +-
>  7 files changed, 274 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit
> -c++11.C
>  create mode 100644 gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit
> -c++98.C
>  create mode 100644 gcc/testsuite/gcc.dg/Wimplicit-fallthrough
> -fixit.c
> 
> diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
> index dc81755..40509f1 100644
> --- a/gcc/diagnostic.c
> +++ b/gcc/diagnostic.c
> @@ -942,7 +942,8 @@ diagnostic_report_diagnostic (diagnostic_context
> *context,
>    diagnostic->x_data = NULL;
>  
>    if (context->edit_context_ptr)
> -    context->edit_context_ptr->add_fixits (diagnostic->richloc);
> +    if (diagnostic->richloc->fixits_can_be_auto_applied_p ())
> +      context->edit_context_ptr->add_fixits (diagnostic->richloc);
>  
>    context->lock--;
>  
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index fd27eb1..19dd6dc 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2038,6 +2038,60 @@ should_warn_for_implicit_fallthrough
> (gimple_stmt_iterator *gsi_p, tree label)
>    return true;
>  }
>  
> +/* Attempt to add a fix-it hint to RICHLOC, suggesting the insertion
> +   of a new line containing LINE_CONTENT (which does not need
> +   a '\n' character).
> +   The new line is to be inserted immediately before the
> +   line containing NEXT, using the location of PREV to determine
> +   indentation.
> +   The new line will only be inserted if there is nothing on
> +   NEXT's line before NEXT, and we can determine the indentation
> +   of PREV.  */
> +
> +static void
> +add_newline_fixit_with_indentation (rich_location *richloc,
> +				    location_t prev,
> +				    location_t next,
> +				    const char *line_content)
> +{
> +  /* Check that the line containing NEXT is blank, up to NEXT.  */
> +  int line_width;
> +  expanded_location exploc_next = expand_location (next);
> +  const char *line
> +    = location_get_source_line (exploc_next.file, exploc_next.line,
> +				&line_width);
> +  if (!line)
> +    return;
> +  if (line_width < exploc_next.column)
> +    return;
> +  /* Columns are 1-based.  */
> +  for (int column = 1; column < exploc_next.column; ++column)
> +    if (!ISSPACE (line[column - 1]))
> +      return;
> +
> +  /* Locate the start of the line containing NEXT.  */
> +  const line_map *map = linemap_lookup (line_table, next);
> +  if (linemap_macro_expansion_map_p (map))
> +    return;
> +  const line_map_ordinary *ordmap = linemap_check_ordinary (map);
> +  location_t start_of_line
> +    = linemap_position_for_line_and_column (line_table, ordmap,
> +					    exploc_next.line, 1);
> +
> +  /* Generate an insertion string, indenting by the amount PREV
> +     was indented.  */
> +  int prev_column = LOCATION_COLUMN (prev);
> +  pretty_printer tmp_pp;
> +  pretty_printer *pp = &tmp_pp;
> +  /* Columns are 1-based.  */
> +  for (int column = 1; column < prev_column; ++column)
> +    pp_space (pp);
> +  pp_string (pp, line_content);
> +  pp_newline (pp);
> +
> +  richloc->add_fixit_insert_before (start_of_line, pp_formatted_text
> (pp));
> +}
> +
>  /* Callback for walk_gimple_seq.  */
>  
>  static tree
> @@ -2111,7 +2165,41 @@ warn_implicit_fallthrough_r
> (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
>  				     OPT_Wimplicit_fallthrough_,
>  				     "this statement may fall
> through");
>  	    if (warned_p)
> -	      inform (gimple_location (next), "here");
> +	      {
> +		inform (gimple_location (next), "here");
> +
> +		/* Suggestion one: add attribute fallthrough.  */
> +		rich_location rloc_attr (line_table, gimple_location
> (next));
> +		const char *attr_hint;
> +		/* For C++17, we'd recommend [[fallthrough]];, but
> we're not
> +		   there yet.  For C++11, recommend
> [[gnu::fallthrough]];.  */
> +		if (strncmp (lang_hooks.name, "GNU C++14", 9) == 0
> +		    || strncmp (lang_hooks.name, "GNU C++11", 9) ==
> 0)
> +		  attr_hint = "[[gnu::fallthrough]];";
> +		else
> +		  /* Otherwise, recommend __attribute__
> ((fallthrough));.  */
> +		  attr_hint = "__attribute__ ((fallthrough));";
> +
> +		add_newline_fixit_with_indentation (&rloc_attr,
> +						    gimple_location
> (prev),
> +						    gimple_location
> (next),
> +						    attr_hint);
> +		rloc_attr.fixits_cannot_be_auto_applied ();
> +		inform_at_rich_loc (&rloc_attr,
> +				    "insert %qs to silence this
> warning",
> +				    attr_hint);
> +
> +		/* Suggestion two: add "break;".  */
> +		rich_location rloc_break (line_table,
> gimple_location (next));
> +		add_newline_fixit_with_indentation (&rloc_break,
> +						    gimple_location
> (prev),
> +						    gimple_location
> (next),
> +						    "break;");
> +		rloc_break.fixits_cannot_be_auto_applied ();
> +		inform_at_rich_loc (&rloc_break,
> +				    "insert %qs to avoid fall
> -through",
> +				    "break;");
> +	      }
>  
>  	    /* Mark this label as processed so as to prevent
> multiple
>  	       warnings in nested switches.  */
> diff --git a/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++11.C
> b/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++11.C
> new file mode 100644
> index 0000000..afc808e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++11.C
> @@ -0,0 +1,43 @@
> +/* PR c/7652 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wimplicit-fallthrough -std=c++11 -fdiagnostics
> -show-caret -fdiagnostics-generate-patch" } */
> +/* We specify -fdiagnostics-generate-patch, but no patch should be
> +   generated, since user-intervention is required to choose between
> +   two different fix-it hints.  */
> +
> +int x;
> +
> +void set_x (char ch, int a, int b)
> +{
> +  switch (ch)
> +    {
> +    case 'a':
> +      x = a; /* { dg-warning "statement may fall through" } */
> +      /* { dg-begin-multiline-output "" }
> +       x = a;
> +       ~~^~~
> +       { dg-end-multiline-output "" } */
> +
> +    case 'b':    /* { dg-line second_case } */
> +      x = b;
> +      /* { dg-message "here" "" { target *-*-* } second_case } */
> +      /* { dg-begin-multiline-output "" }
> +     case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +
> +      /* { dg-message "insert '..gnu::fallthrough..;' to silence
> this warning" "" { target *-*-* } second_case } */
> +      /* { dg-begin-multiline-output "" }
> ++        [[gnu::fallthrough]];
> +     case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +
> +      /* { dg-message "insert 'break;' to avoid fall-through" "" {
> target *-*-* } second_case } */
> +      /* { dg-begin-multiline-output "" }
> ++        break;
> +     case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +    }
> +}
> diff --git a/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++98.C
> b/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++98.C
> new file mode 100644
> index 0000000..8730b4c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/Wimplicit-fallthrough-fixit-c++98.C
> @@ -0,0 +1,43 @@
> +/* PR c/7652 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wimplicit-fallthrough -std=c++98 -fdiagnostics
> -show-caret -fdiagnostics-generate-patch" } */
> +/* We specify -fdiagnostics-generate-patch, but no patch should be
> +   generated, since user-intervention is required to choose between
> +   two different fix-it hints.  */
> +
> +int x;
> +
> +void set_x (char ch, int a, int b)
> +{
> +  switch (ch)
> +    {
> +    case 'a':
> +      x = a; /* { dg-warning "statement may fall through" } */
> +      /* { dg-begin-multiline-output "" }
> +       x = a;
> +       ~~^~~
> +       { dg-end-multiline-output "" } */
> +
> +    case 'b':    /* { dg-line second_case } */
> +      x = b;
> +      /* { dg-message "here" "" { target *-*-* } second_case } */
> +      /* { dg-begin-multiline-output "" }
> +     case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +
> +      /* { dg-message "insert '__attribute__
> \\(\\(fallthrough\\)\\);' to silence this warning" "" { target *-*-*
> } second_case } */
> +      /* { dg-begin-multiline-output "" }
> ++        __attribute__ ((fallthrough));
> +     case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +
> +      /* { dg-message "insert 'break;' to avoid fall-through" "" {
> target *-*-* } second_case } */
> +      /* { dg-begin-multiline-output "" }
> ++        break;
> +     case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +    }
> +}
> diff --git a/gcc/testsuite/gcc.dg/Wimplicit-fallthrough-fixit.c
> b/gcc/testsuite/gcc.dg/Wimplicit-fallthrough-fixit.c
> new file mode 100644
> index 0000000..53daca6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/Wimplicit-fallthrough-fixit.c
> @@ -0,0 +1,73 @@
> +/* PR c/7652 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wimplicit-fallthrough -fdiagnostics-show-caret 
> -fdiagnostics-generate-patch" } */
> +/* We specify -fdiagnostics-generate-patch, but no patch should be
> +   generated, since user-intervention is required to choose between
> +   two different fix-it hints.  */
> +
> +int x;
> +
> +void set_x (char ch, int a, int b)
> +{
> +  switch (ch)
> +    {
> +    case 'a':
> +      x = a; /* { dg-warning "statement may fall through" } */
> +      /* { dg-begin-multiline-output "" }
> +       x = a;
> +       ~~^~~
> +       { dg-end-multiline-output "" } */
> +
> +
> +    case 'b':    /* { dg-line second_case } */
> +      x = b;
> +      /* { dg-message "here" "" { target *-*-* } second_case } */
> +      /* { dg-begin-multiline-output "" }
> +     case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +
> +      /* { dg-message "insert '__attribute__
> \\(\\(fallthrough\\)\\);' to silence this warning" "" { target *-*-*
> } second_case } */
> +      /* { dg-begin-multiline-output "" }
> ++        __attribute__ ((fallthrough));
> +     case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +
> +      /* { dg-message "insert 'break;' to avoid fall-through" "" {
> target *-*-* } second_case } */
> +      /* { dg-begin-multiline-output "" }
> ++        break;
> +     case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +    }
> +}
> +
> +#define FOO
> +
> +void set_x_2 (char ch, int a, int b)
> +{
> +  switch (ch)
> +    {
> +    case 'a':
> +      x = a; /* { dg-warning "statement may fall through" } */
> +      /* { dg-begin-multiline-output "" }
> +       x = a;
> +       ~~^~~
> +       { dg-end-multiline-output "" } */
> +
> +FOO case 'b':    /* { dg-line case_2 } */
> +      x = b;
> +      /* { dg-message "here" "" { target *-*-* } case_2 } */
> +      /* { dg-begin-multiline-output "" }
> + FOO case 'b':
> +     ^~~~
> +	 { dg-end-multiline-output "" } */
> +
> +      /* This case isn't the first thing on its line, so the
> suggestions
> +	 should not provide fix-it hints.  */
> +
> +      /* { dg-message "insert '__attribute__
> \\(\\(fallthrough\\)\\);' to silence this warning" "" { target *-*-*
> } case_2 } */
> +      /* { dg-message "insert 'break;' to avoid fall-through" "" {
> target *-*-* } case_2 } */
> +    }
> +}
> diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> index 90d65d7..be3041d 100644
> --- a/libcpp/include/line-map.h
> +++ b/libcpp/include/line-map.h
> @@ -1663,6 +1663,27 @@ class rich_location
>    fixit_hint *get_last_fixit_hint () const;
>    bool seen_impossible_fixit_p () const { return
> m_seen_impossible_fixit; }
>  
> +  /* Set this if the fix-it hints are not suitable to be
> +     automatically applied.
> +
> +     For example, if you are suggesting more than one
> +     mutually exclusive solution to a problem, then
> +     it doesn't make sense to apply all of the solutions;
> +     manual intervention is required.
> +
> +     If set, then the fix-it hints in the rich_location will
> +     be printed, but will not be added to generated patches,
> +     or affect the modified version of the file.  */
> +  void fixits_cannot_be_auto_applied ()
> +  {
> +    m_fixits_cannot_be_auto_applied = true;
> +  }
> +
> +  bool fixits_can_be_auto_applied_p () const
> +  {
> +    return !m_fixits_cannot_be_auto_applied;
> +  }
> +
>  private:
>    bool reject_impossible_fixit (source_location where);
>    void stop_supporting_fixits ();
> @@ -1686,6 +1707,7 @@ protected:
>    semi_embedded_vec <fixit_hint *, MAX_STATIC_FIXIT_HINTS>
> m_fixit_hints;
>  
>    bool m_seen_impossible_fixit;
> +  bool m_fixits_cannot_be_auto_applied;
>  };
>  
>  /* A fix-it hint: a suggested insertion, replacement, or deletion of
> text.
> diff --git a/libcpp/line-map.c b/libcpp/line-map.c
> index c4b7cb2..5caaf6b 100644
> --- a/libcpp/line-map.c
> +++ b/libcpp/line-map.c
> @@ -2012,7 +2012,8 @@ rich_location::rich_location (line_maps *set,
> source_location loc) :
>    m_column_override (0),
>    m_have_expanded_location (false),
>    m_fixit_hints (),
> -  m_seen_impossible_fixit (false)
> +  m_seen_impossible_fixit (false),
> +  m_fixits_cannot_be_auto_applied (false)
>  {
>    add_range (loc, true);
>  }

Re: [PING] Re: [PATCH] Fix-it hints for -Wimplicit-fallthrough

[Martin Sebor]

On 05/26/2017 01:59 PM, David Malcolm wrote:
> Ping:
>   https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00334.html
>
> On Thu, 2017-05-04 at 14:16 -0400, David Malcolm wrote:
>> As of r247522, fix-it-hints can suggest the insertion of new lines.
>>
>> This patch updates -Wimplicit-fallthrough to provide suggestions
>> with fix-it hints, showing the user where to insert "break;" or
>> fallthrough attributes.
>>
>> For example:
>>
>>  test.c: In function 'set_x':
>>  test.c:15:9: warning: this statement may fall through [-Wimplicit
>> -fallthrough=]
>>         x = a;
>>         ~~^~~
>>  test.c:22:5: note: here
>>       case 'b':
>>       ^~~~
>>  test.c:22:5: note: insert '__attribute__ ((fallthrough));' to
>> silence this warning
>>  +        __attribute__ ((fallthrough));
>>       case 'b':
>>       ^~~~
>>  test.c:22:5: note: insert 'break;' to avoid fall-through
>>  +        break;
>>       case 'b':
>>       ^~~~

I haven't read the patch but the notes above make me wonder:

If the location of at least one of t hints is always the same
as that of the first "note: here" would it make sense to lose
the latter and reduce the size of the output?  (Or lose it in
the cases where one of the fix-it hints does share a location
with it).

Martin

Re: [PING] Re: [PATCH] Fix-it hints for -Wimplicit-fallthrough

[Marek Polacek]

On Fri, May 26, 2017 at 02:13:56PM -0600, Martin Sebor wrote:
> On 05/26/2017 01:59 PM, David Malcolm wrote:
> > Ping:
> >   https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00334.html
> > 
> > On Thu, 2017-05-04 at 14:16 -0400, David Malcolm wrote:
> > > As of r247522, fix-it-hints can suggest the insertion of new lines.
> > > 
> > > This patch updates -Wimplicit-fallthrough to provide suggestions
> > > with fix-it hints, showing the user where to insert "break;" or
> > > fallthrough attributes.
> > > 
> > > For example:
> > > 
> > >  test.c: In function 'set_x':
> > >  test.c:15:9: warning: this statement may fall through [-Wimplicit
> > > -fallthrough=]
> > >         x = a;
> > >         ~~^~~
> > >  test.c:22:5: note: here
> > >       case 'b':
> > >       ^~~~
> > >  test.c:22:5: note: insert '__attribute__ ((fallthrough));' to
> > > silence this warning
> > >  +        __attribute__ ((fallthrough));
> > >       case 'b':
> > >       ^~~~
> > >  test.c:22:5: note: insert 'break;' to avoid fall-through
> > >  +        break;
> > >       case 'b':
> > >       ^~~~
> 
> I haven't read the patch but the notes above make me wonder:
> 
> If the location of at least one of t hints is always the same
> as that of the first "note: here" would it make sense to lose
> the latter and reduce the size of the output?  (Or lose it in
> the cases where one of the fix-it hints does share a location
> with it).

I agree that it's a tad verbose but I'm not sure if it'd be easy
to suppress printing
     case 2:
     ^~~~
more times simple enough.  So I'd be fine with the current state.
I'd also be happy with e.g.

a.c: In function ‘foo’:
a.c:7:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
       x = 1;
       ~~^~~
a.c:8:5: note: here
     case 2:
     ^~~~
a.c:8:5: note: insert ‘__attribute__ ((fallthrough));’ to silence this warning
+        __attribute__ ((fallthrough));
	 or insert ‘break;’ to avoid fall-through
+        break;

but I don't think we've got the means to do so.

On the patch, I'd think that add_newline_fixit_with_indentation doesn't
belong to gimplify.c, even though it only has one user.  But I won't be
able to approve it in any case.

	Marek

Re: [PATCH] Fix-it hints for -Wimplicit-fallthrough

[Jeff Law]

On 05/04/2017 12:16 PM, David Malcolm wrote:
> As of r247522, fix-it-hints can suggest the insertion of new lines.
> 
> This patch updates -Wimplicit-fallthrough to provide suggestions
> with fix-it hints, showing the user where to insert "break;" or
> fallthrough attributes.
> 
> For example:
> 
>  test.c: In function 'set_x':
>  test.c:15:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
>         x = a;
>         ~~^~~
>  test.c:22:5: note: here
>       case 'b':
>       ^~~~
>  test.c:22:5: note: insert '__attribute__ ((fallthrough));' to silence this warning
>  +        __attribute__ ((fallthrough));
>       case 'b':
>       ^~~~
>  test.c:22:5: note: insert 'break;' to avoid fall-through
>  +        break;
>       case 'b':
>       ^~~~
> 
> The idea is that if an IDE supports -fdiagnostics-parseable-fixits, the
> user can fix these issues by clicking on them.
> 
> It's loosely based on part of Marek's patch:
>   https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01948.html
> but with extra logic for locating a suitable place to insert the
> fix-it hint, so that, if possible, it is on a line by itself before
> the "case", indented the same as the prior statement.  If that's not
> possible, no fix-it hint is emitted.
> 
> In Marek's patch he wrote:
>   /* For C++17, we'd recommend [[fallthrough]];, but we're not
>      there yet.  For C++11, recommend [[gnu::fallthrough]];.  */
> "[[fallthrough]]" appears to work, but it appears that lang_hooks.name
> doesn't expose C++17 yet, so the patch recommends [[gnu::fallthrough]
> for C++17.
> 
> 
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> 
> OK for trunk?
> 
> gcc/ChangeLog:
> 	PR c/7652
> 	* diagnostic.c (diagnostic_report_diagnostic): Only add fixits
> 	to the edit_context if they can be auto-applied.
> 	* gimplify.c (add_newline_fixit_with_indentation): New function.
> 	(warn_implicit_fallthrough_r): Add suggestions on how to fix
> 	-Wimplicit-fallthrough.
> 
> gcc/testsuite/ChangeLog:
> 	PR c/7652
> 	* g++.dg/Wimplicit-fallthrough-fixit-c++11.C: New test case.
> 	* g++.dg/Wimplicit-fallthrough-fixit-c++98.C: New test case.
> 	* gcc.dg/Wimplicit-fallthrough-fixit.c: New test case.
> 
> libcpp/ChangeLog:
> 	PR c/7652
> 	* include/line-map.h
> 	(rich_location::fixits_cannot_be_auto_applied): New method.
> 	(rich_location::fixits_can_be_auto_applied_p): New accessor.
> 	(rich_location::m_fixits_cannot_be_auto_applied): New field.
> 	* line-map.c (rich_location::rich_location): Initialize new field.
What's the state on this?  It looks like the diagnostic.c change is in,
but others are not?

Jeff