* [PATCH] Fix-it hints for -Wimplicit-fallthrough
@ 2017-05-04 17:46 David Malcolm
2017-05-26 20:13 ` [PING] " David Malcolm
2017-06-23 4:38 ` Jeff Law
0 siblings, 2 replies; 5+ messages in thread
From: David Malcolm @ 2017-05-04 17:46 UTC (permalink / raw)
To: gcc-patches; +Cc: Marek Polacek, 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®rtested 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PING] Re: [PATCH] Fix-it hints for -Wimplicit-fallthrough
2017-05-04 17:46 [PATCH] Fix-it hints for -Wimplicit-fallthrough David Malcolm
@ 2017-05-26 20:13 ` David Malcolm
2017-05-26 21:13 ` Martin Sebor
2017-06-23 4:38 ` Jeff Law
1 sibling, 1 reply; 5+ messages in thread
From: David Malcolm @ 2017-05-26 20:13 UTC (permalink / raw)
To: gcc-patches; +Cc: Marek Polacek
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®rtested 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);
> }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PING] Re: [PATCH] Fix-it hints for -Wimplicit-fallthrough
2017-05-26 20:13 ` [PING] " David Malcolm
@ 2017-05-26 21:13 ` Martin Sebor
2017-06-06 13:12 ` Marek Polacek
0 siblings, 1 reply; 5+ messages in thread
From: Martin Sebor @ 2017-05-26 21:13 UTC (permalink / raw)
To: David Malcolm, gcc-patches; +Cc: Marek Polacek
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PING] Re: [PATCH] Fix-it hints for -Wimplicit-fallthrough
2017-05-26 21:13 ` Martin Sebor
@ 2017-06-06 13:12 ` Marek Polacek
0 siblings, 0 replies; 5+ messages in thread
From: Marek Polacek @ 2017-06-06 13:12 UTC (permalink / raw)
To: Martin Sebor; +Cc: David Malcolm, gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix-it hints for -Wimplicit-fallthrough
2017-05-04 17:46 [PATCH] Fix-it hints for -Wimplicit-fallthrough David Malcolm
2017-05-26 20:13 ` [PING] " David Malcolm
@ 2017-06-23 4:38 ` Jeff Law
1 sibling, 0 replies; 5+ messages in thread
From: Jeff Law @ 2017-06-23 4:38 UTC (permalink / raw)
To: David Malcolm, gcc-patches; +Cc: Marek Polacek
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®rtested 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-23 4:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-04 17:46 [PATCH] Fix-it hints for -Wimplicit-fallthrough David Malcolm
2017-05-26 20:13 ` [PING] " David Malcolm
2017-05-26 21:13 ` Martin Sebor
2017-06-06 13:12 ` Marek Polacek
2017-06-23 4:38 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).