From 2ffe0809cb3e9a49387a4657dea65a287b377617 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Fri, 19 Aug 2016 21:18:05 +0000 Subject: [PATCH] Reimplement removal fix-it hints in terms of replace This patch eliminates class fixit_remove, reimplementing rich_location::add_fixit_remove in terms of replacement with the empty string. Deleting the removal subclass simplifies fixit-handling code, as we only have two concrete fixit_hint subclasses to deal with, rather than three. The patch also fixes some problems in diagnostic-show-locus.c for situations where a replacement fix-it has a different range to the range of the diagnostic, by unifying the drawing of the two kinds of fixits. For example, this: foo = bar.field; ^ m_field becomes: foo = bar.field; ^ ----- m_field showing the range to be replaced. gcc/ChangeLog: * diagnostic-show-locus.c (layout::annotation_line_showed_range_p): New method. (layout::print_any_fixits): Remove case fixit_hint::REMOVE. Reimplement case fixit_hint::REPLACE to cover removals, and replacements where the range of the replacement isn't one of the ranges in the rich_location. (test_one_liner_fixit_replace): Likewise. (selftest::test_one_liner_fixit_replace_non_equal_range): New function. (selftest::test_one_liner_fixit_replace_equal_secondary_range): New function. (selftest::test_diagnostic_show_locus_one_liner): Call the new functions. * diagnostic.c (print_parseable_fixits): Remove case fixit_hint::REMOVE. libcpp/ChangeLog: * include/line-map.h (fixit_hint::kind): Delete REPLACE. (class fixit_remove): Delete. * line-map.c (rich_location::add_fixit_remove): Reimplement by calling add_fixit_replace with an empty string. (fixit_remove::fixit_remove): Delete. (fixit_remove::affects_line_p): Delete. From-SVN: r239632 --- gcc/ChangeLog | 18 ++++++ gcc/diagnostic-show-locus.c | 124 ++++++++++++++++++++++++++++++------ gcc/diagnostic.c | 4 -- libcpp/ChangeLog | 9 +++ libcpp/include/line-map.h | 23 +------ libcpp/line-map.c | 18 +----- 6 files changed, 133 insertions(+), 63 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 659330073c8..02c85f893f8 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,21 @@ +2016-08-19 David Malcolm + + * diagnostic-show-locus.c + (layout::annotation_line_showed_range_p): New method. + (layout::print_any_fixits): Remove case fixit_hint::REMOVE. + Reimplement case fixit_hint::REPLACE to cover removals, and + replacements where the range of the replacement isn't one + of the ranges in the rich_location. + (test_one_liner_fixit_replace): Likewise. + (selftest::test_one_liner_fixit_replace_non_equal_range): New + function. + (selftest::test_one_liner_fixit_replace_equal_secondary_range): + New function. + (selftest::test_diagnostic_show_locus_one_liner): Call the new + functions. + * diagnostic.c (print_parseable_fixits): Remove case + fixit_hint::REMOVE. + 2016-08-19 Uros Bizjak PR target/77270 diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index 4498f7ce5da..32b107833a8 100644 --- a/gcc/diagnostic-show-locus.c +++ b/gcc/diagnostic-show-locus.c @@ -199,6 +199,8 @@ class layout bool print_source_line (int row, line_bounds *lbounds_out); void print_annotation_line (int row, const line_bounds lbounds); + bool annotation_line_showed_range_p (int line, int start_column, + int finish_column) const; void print_any_fixits (int row, const rich_location *richloc); void show_ruler (int max_column) const; @@ -1053,6 +1055,26 @@ layout::print_annotation_line (int row, const line_bounds lbounds) print_newline (); } +/* Subroutine of layout::print_any_fixits. + + Determine if the annotation line printed for LINE contained + the exact range from START_COLUMN to FINISH_COLUMN. */ + +bool +layout::annotation_line_showed_range_p (int line, int start_column, + int finish_column) const +{ + layout_range *range; + int i; + FOR_EACH_VEC_ELT (m_layout_ranges, i, range) + if (range->m_start.m_line == line + && range->m_start.m_column == start_column + && range->m_finish.m_line == line + && range->m_finish.m_column == finish_column) + return true; + return false; +} + /* If there are any fixit hints on source line ROW within RICHLOC, print them. They are printed in order, attempting to combine them onto lines, but starting new lines if necessary. */ @@ -1083,33 +1105,39 @@ layout::print_any_fixits (int row, const rich_location *richloc) } break; - case fixit_hint::REMOVE: + case fixit_hint::REPLACE: { - fixit_remove *remove = static_cast (hint); - /* This assumes the removal just affects one line. */ - source_range src_range = remove->get_range (); + fixit_replace *replace = static_cast (hint); + source_range src_range = replace->get_range (); + int line = LOCATION_LINE (src_range.m_start); int start_column = LOCATION_COLUMN (src_range.m_start); int finish_column = LOCATION_COLUMN (src_range.m_finish); - move_to_column (&column, start_column); - for (int column = start_column; column <= finish_column; column++) + + /* If the range of the replacement wasn't printed in the + annotation line, then print an extra underline to + indicate exactly what is being replaced. + Always show it for removals. */ + if (!annotation_line_showed_range_p (line, start_column, + finish_column) + || replace->get_length () == 0) { + move_to_column (&column, start_column); m_colorizer.set_fixit_hint (); - pp_character (m_pp, '-'); + for (; column <= finish_column; column++) + pp_character (m_pp, '-'); m_colorizer.set_normal_text (); } - } - break; - - case fixit_hint::REPLACE: - { - fixit_replace *replace = static_cast (hint); - int start_column - = LOCATION_COLUMN (replace->get_range ().m_start); - move_to_column (&column, start_column); - m_colorizer.set_fixit_hint (); - pp_string (m_pp, replace->get_string ()); - m_colorizer.set_normal_text (); - column += replace->get_length (); + /* Print the replacement text. REPLACE also covers + removals, so only do this extra work (potentially starting + a new line) if we have actual replacement text. */ + if (replace->get_length () > 0) + { + move_to_column (&column, start_column); + m_colorizer.set_fixit_hint (); + pp_string (m_pp, replace->get_string ()); + m_colorizer.set_normal_text (); + column += replace->get_length (); + } } break; @@ -1502,6 +1530,60 @@ test_one_liner_fixit_replace () pp_formatted_text (dc.printer)); } +/* Replace fix-it hint: replacing "field" with "m_field", + but where the caret was elsewhere. */ + +static void +test_one_liner_fixit_replace_non_equal_range () +{ + test_diagnostic_context dc; + location_t equals = linemap_position_for_column (line_table, 5); + location_t start = linemap_position_for_column (line_table, 11); + location_t finish = linemap_position_for_column (line_table, 15); + rich_location richloc (line_table, equals); + source_range range; + range.m_start = start; + range.m_finish = finish; + richloc.add_fixit_replace (range, "m_field"); + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + /* The replacement range is not indicated in the annotation line, so + it should be indicated via an additional underline. */ + ASSERT_STREQ ("\n" + " foo = bar.field;\n" + " ^\n" + " -----\n" + " m_field\n", + pp_formatted_text (dc.printer)); +} + +/* Replace fix-it hint: replacing "field" with "m_field", + where the caret was elsewhere, but where a secondary range + exactly covers "field". */ + +static void +test_one_liner_fixit_replace_equal_secondary_range () +{ + test_diagnostic_context dc; + location_t equals = linemap_position_for_column (line_table, 5); + location_t start = linemap_position_for_column (line_table, 11); + location_t finish = linemap_position_for_column (line_table, 15); + rich_location richloc (line_table, equals); + location_t field = make_location (start, start, finish); + richloc.add_range (field, false); + source_range range; + range.m_start = start; + range.m_finish = finish; + richloc.add_fixit_replace (range, "m_field"); + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + /* The replacement range is indicated in the annotation line, + so it shouldn't be indicated via an additional underline. */ + ASSERT_STREQ ("\n" + " foo = bar.field;\n" + " ^ ~~~~~\n" + " m_field\n", + pp_formatted_text (dc.printer)); +} + /* Run the various one-liner tests. */ static void @@ -1532,6 +1614,8 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_) test_one_liner_fixit_insert (); test_one_liner_fixit_remove (); test_one_liner_fixit_replace (); + test_one_liner_fixit_replace_non_equal_range (); + test_one_liner_fixit_replace_equal_secondary_range (); } /* Run all of the selftests within this file. */ diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c index fec48c4e441..b47da383fc3 100644 --- a/gcc/diagnostic.c +++ b/gcc/diagnostic.c @@ -758,10 +758,6 @@ print_parseable_fixits (pretty_printer *pp, rich_location *richloc) } break; - case fixit_hint::REMOVE: - print_escaped_string (pp, ""); - break; - case fixit_hint::REPLACE: { const fixit_replace *replace diff --git a/libcpp/ChangeLog b/libcpp/ChangeLog index f680f8bc899..b0fd9b52ed8 100644 --- a/libcpp/ChangeLog +++ b/libcpp/ChangeLog @@ -1,3 +1,12 @@ +2016-08-19 David Malcolm + + * include/line-map.h (fixit_hint::kind): Delete REPLACE. + (class fixit_remove): Delete. + * line-map.c (rich_location::add_fixit_remove): Reimplement + by calling add_fixit_replace with an empty string. + (fixit_remove::fixit_remove): Delete. + (fixit_remove::affects_line_p): Delete. + 2016-08-19 Joseph Myers PR c/32187 diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index 443086a0b70..f65931c5e52 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -1422,7 +1422,7 @@ protected: class fixit_hint { public: - enum kind {INSERT, REMOVE, REPLACE}; + enum kind {INSERT, REPLACE}; virtual ~fixit_hint () {} @@ -1453,27 +1453,6 @@ class fixit_insert : public fixit_hint size_t m_len; }; -class fixit_remove : public fixit_hint -{ - public: - fixit_remove (source_range src_range); - ~fixit_remove () {} - - enum kind get_kind () const { return REMOVE; } - bool affects_line_p (const char *file, int line); - source_location get_start_loc () const { return m_src_range.m_start; } - bool maybe_get_end_loc (source_location *out) const - { - *out = m_src_range.m_finish; - return true; - } - - source_range get_range () const { return m_src_range; } - - private: - source_range m_src_range; -}; - class fixit_replace : public fixit_hint { public: diff --git a/libcpp/line-map.c b/libcpp/line-map.c index 141af9d2cde..3890eff7ba0 100644 --- a/libcpp/line-map.c +++ b/libcpp/line-map.c @@ -2086,8 +2086,7 @@ rich_location::add_fixit_insert (source_location where, void rich_location::add_fixit_remove (source_range src_range) { - linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS); - m_fixit_hints[m_num_fixit_hints++] = new fixit_remove (src_range); + add_fixit_replace (src_range, ""); } /* Add a fixit-hint, suggesting replacement of the content at @@ -2130,21 +2129,6 @@ fixit_insert::affects_line_p (const char *file, int line) return false; } -/* class fixit_remove. */ - -fixit_remove::fixit_remove (source_range src_range) -: m_src_range (src_range) -{ -} - -/* Implementation of fixit_hint::affects_line_p for fixit_remove. */ - -bool -fixit_remove::affects_line_p (const char *file, int line) -{ - return m_src_range.intersects_line_p (file, line); -} - /* class fixit_replace. */ fixit_replace::fixit_replace (source_range src_range, -- 2.30.2