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); > }