From 3d4f9f878d9aae137be3151920dfcde089a28ddc Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Wed, 31 Aug 2016 18:54:55 +0000 Subject: [PATCH] diagnostic-show-locus.c: handle fixits on lines outside the regular ranges The diagnostic_show_locus implementation determines the set of line spans that need printing based on the ranges within the rich_location (in layout::calculate_line_spans). Currently this doesn't take into account fix-it hints, and hence we fail to print fix-it hints that are on lines outside of those ranges. This patch updates the implementation to take fix-it hints into account when calculating the pertinent line spans, so that such fix-it hints do get printed. It also adds some validation, to ensure that we don't attempt to print fix-its hints affecting a different source file. gcc/ChangeLog: * diagnostic-show-locus.c (class layout): Add field m_fixit_hints. (layout_range::intersects_line_p): New method. (test_range_contains_point_for_single_point): Rename to... (test_layout_range_for_single_point): ...this, and add testing for layout_range::intersects_line_p. (test_range_contains_point_for_single_line): Rename to... (test_layout_range_for_single_line): ...this, and add testing for layout_range::intersects_line_p. (test_range_contains_point_for_multiple_lines): Rename to... (test_layout_range_for_multiple_lines): ...this, and add testing for layout_range::intersects_line_p. (layout::layout): Populate m_fixit_hints. (layout::get_expanded_location): Handle the case of a line-span for a fix-it hint. (layout::validate_fixit_hint_p): New method. (get_line_span_for_fixit_hint): New function. (layout::calculate_line_spans): Add spans for fixit-hints. (layout::should_print_annotation_line_p): New method. (layout::print_any_fixits): Drop param "richloc", instead using validated fixits in m_fixit_hints. Add "const" to hint pointers. (diagnostic_show_locus): Avoid printing blank annotation lines. (selftest::test_diagnostic_context::test_diagnostic_context): Initialize show_column and start_span. (selftest::test_diagnostic_context::start_span_cb): New static function. (selftest::test_diagnostic_show_locus_fixit_lines): New function. (selftest::diagnostic_show_locus_c_tests): Update for function renamings. Call test_diagnostic_show_locus_fixit_lines. libcpp/ChangeLog: * include/line-map.h (class fixit_remove): Remove stray decl. (fixit_hint::affects_line_p): Make const. (fixit_insert::affects_line_p): Likewise. (fixit_replace::affects_line_p): Likewise. * line-map.c (fixit_insert::affects_line_p): Likewise. (fixit_replace::affects_line_p): Likewise. From-SVN: r239906 --- gcc/ChangeLog | 31 ++++ gcc/diagnostic-show-locus.c | 292 +++++++++++++++++++++++++++++++++--- libcpp/ChangeLog | 9 ++ libcpp/include/line-map.h | 7 +- libcpp/line-map.c | 4 +- 5 files changed, 315 insertions(+), 28 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 2d508331e50..29506594b8b 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,34 @@ +2016-08-31 David Malcolm + + * diagnostic-show-locus.c (class layout): Add field m_fixit_hints. + (layout_range::intersects_line_p): New method. + (test_range_contains_point_for_single_point): Rename to... + (test_layout_range_for_single_point): ...this, and add testing + for layout_range::intersects_line_p. + (test_range_contains_point_for_single_line): Rename to... + (test_layout_range_for_single_line): ...this, and add testing + for layout_range::intersects_line_p. + (test_range_contains_point_for_multiple_lines): Rename to... + (test_layout_range_for_multiple_lines): ...this, and add testing + for layout_range::intersects_line_p. + (layout::layout): Populate m_fixit_hints. + (layout::get_expanded_location): Handle the case of a line-span + for a fix-it hint. + (layout::validate_fixit_hint_p): New method. + (get_line_span_for_fixit_hint): New function. + (layout::calculate_line_spans): Add spans for fixit-hints. + (layout::should_print_annotation_line_p): New method. + (layout::print_any_fixits): Drop param "richloc", instead using + validated fixits in m_fixit_hints. Add "const" to hint pointers. + (diagnostic_show_locus): Avoid printing blank annotation lines. + (selftest::test_diagnostic_context::test_diagnostic_context): + Initialize show_column and start_span. + (selftest::test_diagnostic_context::start_span_cb): New static + function. + (selftest::test_diagnostic_show_locus_fixit_lines): New function. + (selftest::diagnostic_show_locus_c_tests): Update for function + renamings. Call test_diagnostic_show_locus_fixit_lines. + 2016-08-31 Marc Glisse PR tree-optimization/73714 diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index a22a660543a..00a95a19cc0 100644 --- a/gcc/diagnostic-show-locus.c +++ b/gcc/diagnostic-show-locus.c @@ -129,6 +129,7 @@ class layout_range const expanded_location *caret_exploc); bool contains_point (int row, int column) const; + bool intersects_line_p (int row) const; layout_point m_start; layout_point m_finish; @@ -203,14 +204,17 @@ class layout expanded_location get_expanded_location (const line_span *) const; bool print_source_line (int row, line_bounds *lbounds_out); + bool should_print_annotation_line_p (int row) const; 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 print_any_fixits (int row); void show_ruler (int max_column) const; private: + bool validate_fixit_hint_p (const fixit_hint *hint); + void calculate_line_spans (); void print_newline (); @@ -237,6 +241,7 @@ class layout colorizer m_colorizer; bool m_colorize_source_p; auto_vec m_layout_ranges; + auto_vec m_fixit_hints; auto_vec m_line_spans; int m_x_offset; }; @@ -460,9 +465,22 @@ layout_range::contains_point (int row, int column) const return column <= m_finish.m_column; } +/* Does this layout_range contain any part of line ROW? */ + +bool +layout_range::intersects_line_p (int row) const +{ + gcc_assert (m_start.m_line <= m_finish.m_line); + if (row < m_start.m_line) + return false; + if (row > m_finish.m_line) + return false; + return true; +} + #if CHECKING_P -/* A helper function for testing layout_range::contains_point. */ +/* A helper function for testing layout_range. */ static layout_range make_range (int start_line, int start_col, int end_line, int end_col) @@ -475,16 +493,19 @@ make_range (int start_line, int start_col, int end_line, int end_col) &start_exploc); } -/* Selftests for layout_range::contains_point. */ +/* Selftests for layout_range::contains_point and + layout_range::intersects_line_p. */ -/* Selftest for layout_range::contains_point where the layout_range +/* Selftest for layout_range, where the layout_range is a range with start==end i.e. a single point. */ static void -test_range_contains_point_for_single_point () +test_layout_range_for_single_point () { layout_range point = make_range (7, 10, 7, 10); + /* Tests for layout_range::contains_point. */ + /* Before the line. */ ASSERT_FALSE (point.contains_point (6, 1)); @@ -499,16 +520,23 @@ test_range_contains_point_for_single_point () /* After the line. */ ASSERT_FALSE (point.contains_point (8, 1)); + + /* Tests for layout_range::intersects_line_p. */ + ASSERT_FALSE (point.intersects_line_p (6)); + ASSERT_TRUE (point.intersects_line_p (7)); + ASSERT_FALSE (point.intersects_line_p (8)); } -/* Selftest for layout_range::contains_point where the layout_range +/* Selftest for layout_range, where the layout_range is the single-line range shown as "Example A" above. */ static void -test_range_contains_point_for_single_line () +test_layout_range_for_single_line () { layout_range example_a = make_range (2, 22, 2, 38); + /* Tests for layout_range::contains_point. */ + /* Before the line. */ ASSERT_FALSE (example_a.contains_point (1, 1)); @@ -529,16 +557,23 @@ test_range_contains_point_for_single_line () /* After the line. */ ASSERT_FALSE (example_a.contains_point (2, 39)); + + /* Tests for layout_range::intersects_line_p. */ + ASSERT_FALSE (example_a.intersects_line_p (1)); + ASSERT_TRUE (example_a.intersects_line_p (2)); + ASSERT_FALSE (example_a.intersects_line_p (3)); } -/* Selftest for layout_range::contains_point where the layout_range +/* Selftest for layout_range, where the layout_range is the multi-line range shown as "Example B" above. */ static void -test_range_contains_point_for_multiple_lines () +test_layout_range_for_multiple_lines () { layout_range example_b = make_range (3, 14, 5, 8); + /* Tests for layout_range::contains_point. */ + /* Before first line. */ ASSERT_FALSE (example_b.contains_point (1, 1)); @@ -573,6 +608,13 @@ test_range_contains_point_for_multiple_lines () /* After the line. */ ASSERT_FALSE (example_b.contains_point (6, 1)); + + /* Tests for layout_range::intersects_line_p. */ + ASSERT_FALSE (example_b.intersects_line_p (2)); + ASSERT_TRUE (example_b.intersects_line_p (3)); + ASSERT_TRUE (example_b.intersects_line_p (4)); + ASSERT_TRUE (example_b.intersects_line_p (5)); + ASSERT_FALSE (example_b.intersects_line_p (6)); } #endif /* #if CHECKING_P */ @@ -709,7 +751,7 @@ compatible_locations_p (location_t loc_a, location_t loc_b) /* Constructor for class layout. Filter the ranges from the rich_location to those that we can - sanely print, populating m_layout_ranges. + sanely print, populating m_layout_ranges and m_fixit_hints. Determine the range of lines that we will print, splitting them up into an ordered list of disjoint spans of contiguous line numbers. Determine m_x_offset, to ensure that the primary caret @@ -725,6 +767,7 @@ layout::layout (diagnostic_context * context, m_colorizer (context, diagnostic_kind), m_colorize_source_p (context->colorize_source_p), m_layout_ranges (richloc->get_num_locations ()), + m_fixit_hints (richloc->get_num_fixit_hints ()), m_line_spans (1 + richloc->get_num_locations ()), m_x_offset (0) { @@ -798,6 +841,15 @@ layout::layout (diagnostic_context * context, m_layout_ranges.safe_push (ri); } + /* Populate m_fixit_hints, filtering to only those that are in the + same file. */ + for (unsigned int i = 0; i < richloc->get_num_fixit_hints (); i++) + { + const fixit_hint *hint = richloc->get_fixit_hint (i); + if (validate_fixit_hint_p (hint)) + m_fixit_hints.safe_push (hint); + } + /* Populate m_line_spans. */ calculate_line_spans (); @@ -867,12 +919,92 @@ layout::get_expanded_location (const line_span *line_span) const } } + /* Otherwise, use the location of the first fixit-hint present within + the line_span. */ + for (unsigned int i = 0; i < m_fixit_hints.length (); i++) + { + const fixit_hint *hint = m_fixit_hints[i]; + location_t loc = hint->get_start_loc (); + expanded_location exploc = expand_location (loc); + if (line_span->contains_line_p (exploc.line)) + return exploc; + } + /* It should not be possible to have a line span that didn't - contain any of the layout_range instances. */ + contain any of the layout_range or fixit_hint instances. */ gcc_unreachable (); return m_exploc; } +/* Determine if HINT is meaningful to print within this layout. */ + +bool +layout::validate_fixit_hint_p (const fixit_hint *hint) +{ + switch (hint->get_kind ()) + { + case fixit_hint::INSERT: + { + const fixit_insert *insert = static_cast (hint); + location_t loc = insert->get_location (); + if (LOCATION_FILE (loc) != m_exploc.file) + return false; + } + break; + + case fixit_hint::REPLACE: + { + const fixit_replace *replace + = static_cast (hint); + source_range src_range = replace->get_range (); + if (LOCATION_FILE (src_range.m_start) != m_exploc.file) + return false; + if (LOCATION_FILE (src_range.m_finish) != m_exploc.file) + return false; + } + break; + + default: + gcc_unreachable (); + } + + return true; +} + +/* Determine the range of lines affected by HINT. + This assumes that HINT has already been filtered by + validate_fixit_hint_p, and so affects the correct source file. */ + +static line_span +get_line_span_for_fixit_hint (const fixit_hint *hint) +{ + gcc_assert (hint); + switch (hint->get_kind ()) + { + case fixit_hint::INSERT: + { + const fixit_insert *insert = static_cast (hint); + location_t loc = insert->get_location (); + int line = LOCATION_LINE (loc); + return line_span (line, line); + } + break; + + case fixit_hint::REPLACE: + { + const fixit_replace *replace + = static_cast (hint); + source_range src_range = replace->get_range (); + return line_span (LOCATION_LINE (src_range.m_start), + LOCATION_LINE (src_range.m_finish)); + } + break; + + default: + gcc_unreachable (); + } +} + /* We want to print the pertinent source code at a diagnostic. The rich_location can contain multiple locations. This will have been filtered into m_exploc (the caret for the primary location) and @@ -918,6 +1050,14 @@ layout::calculate_line_spans () lr->m_finish.m_line)); } + /* Also add spans for any fix-it hints, in case they cover other lines. */ + for (unsigned int i = 0; i < m_fixit_hints.length (); i++) + { + const fixit_hint *hint = m_fixit_hints[i]; + gcc_assert (hint); + tmp_spans.safe_push (get_line_span_for_fixit_hint (hint)); + } + /* Sort them. */ tmp_spans.qsort(line_span::comparator); @@ -1031,6 +1171,20 @@ layout::print_source_line (int row, line_bounds *lbounds_out) return true; } +/* Determine if we should print an annotation line for ROW. + i.e. if any of m_layout_ranges contains ROW. */ + +bool +layout::should_print_annotation_line_p (int row) const +{ + layout_range *range; + int i; + FOR_EACH_VEC_ELT (m_layout_ranges, i, range) + if (range->intersects_line_p (row)) + return true; + return false; +} + /* Print a line consisting of the caret/underlines for the given source line. */ @@ -1096,17 +1250,17 @@ layout::annotation_line_showed_range_p (int line, int start_column, return false; } -/* If there are any fixit hints on source line ROW within RICHLOC, print them. +/* If there are any fixit hints on source line ROW, print them. They are printed in order, attempting to combine them onto lines, but starting new lines if necessary. */ void -layout::print_any_fixits (int row, const rich_location *richloc) +layout::print_any_fixits (int row) { int column = 0; - for (unsigned int i = 0; i < richloc->get_num_fixit_hints (); i++) + for (unsigned int i = 0; i < m_fixit_hints.length (); i++) { - fixit_hint *hint = richloc->get_fixit_hint (i); + const fixit_hint *hint = m_fixit_hints[i]; if (hint->affects_line_p (m_exploc.file, row)) { /* For now we assume each fixit hint can only touch one line. */ @@ -1114,7 +1268,8 @@ layout::print_any_fixits (int row, const rich_location *richloc) { case fixit_hint::INSERT: { - fixit_insert *insert = static_cast (hint); + const fixit_insert *insert + = static_cast (hint); /* This assumes the insertion just affects one line. */ int start_column = LOCATION_COLUMN (insert->get_location ()); @@ -1128,7 +1283,8 @@ layout::print_any_fixits (int row, const rich_location *richloc) case fixit_hint::REPLACE: { - fixit_replace *replace = static_cast (hint); + const 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); @@ -1370,8 +1526,9 @@ diagnostic_show_locus (diagnostic_context * context, line_bounds lbounds; if (layout.print_source_line (row, &lbounds)) { - layout.print_annotation_line (row, lbounds); - layout.print_any_fixits (row, richloc); + if (layout.should_print_annotation_line_p (row)) + layout.print_annotation_line (row, lbounds); + layout.print_any_fixits (row); } } } @@ -1395,11 +1552,22 @@ class test_diagnostic_context : public diagnostic_context { diagnostic_initialize (this, 0); show_caret = true; + show_column = true; + start_span = start_span_cb; } ~test_diagnostic_context () { diagnostic_finish (this); } + + /* Implementation of diagnostic_start_span_fn, hiding the + real filename (to avoid printing the names of tempfiles). */ + static void + start_span_cb (diagnostic_context *context, expanded_location exploc) + { + exploc.file = "FILENAME"; + default_diagnostic_start_span_fn (context, exploc); + } }; /* Verify that diagnostic_show_locus works sanely on UNKNOWN_LOCATION. */ @@ -1739,6 +1907,85 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_) test_one_liner_many_fixits (); } +/* Verify that we print fixits even if they only affect lines + outside those covered by the ranges in the rich_location. */ + +static void +test_diagnostic_show_locus_fixit_lines (const line_table_case &case_) +{ + /* Create a tempfile and write some text to it. + ...000000000111111111122222222223333333333. + ...123456789012345678901234567890123456789. */ + const char *content + = ("struct point { double x; double y; };\n" /* line 1. */ + "struct point origin = {x: 0.0,\n" /* line 2. */ + " y\n" /* line 3. */ + "\n" /* line 4. */ + "\n" /* line 5. */ + " : 0.0};\n"); /* line 6. */ + temp_source_file tmp (SELFTEST_LOCATION, ".c", content); + line_table_test ltt (case_); + + const line_map_ordinary *ord_map + = linemap_check_ordinary (linemap_add (line_table, LC_ENTER, false, + tmp.get_filename (), 0)); + + linemap_line_start (line_table, 1, 100); + + const location_t final_line_end + = linemap_position_for_line_and_column (line_table, ord_map, 6, 36); + + /* Don't attempt to run the tests if column data might be unavailable. */ + if (final_line_end > LINE_MAP_MAX_LOCATION_WITH_COLS) + return; + + /* A pair of tests for modernizing the initializers to C99-style. */ + + /* The one-liner case (line 2). */ + { + test_diagnostic_context dc; + const location_t x + = linemap_position_for_line_and_column (line_table, ord_map, 2, 24); + const location_t colon + = linemap_position_for_line_and_column (line_table, ord_map, 2, 25); + rich_location richloc (line_table, colon); + richloc.add_fixit_insert (x, "."); + richloc.add_fixit_replace (colon, "="); + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + ASSERT_STREQ ("\n" + " struct point origin = {x: 0.0,\n" + " ^\n" + " .=\n", + pp_formatted_text (dc.printer)); + } + + /* The multiline case. The caret for the rich_location is on line 6; + verify that insertion fixit on line 3 is still printed (and that + span starts are printed due to the gap between the span at line 3 + and that at line 6). */ + { + test_diagnostic_context dc; + const location_t y + = linemap_position_for_line_and_column (line_table, ord_map, 3, 24); + const location_t colon + = linemap_position_for_line_and_column (line_table, ord_map, 6, 25); + rich_location richloc (line_table, colon); + richloc.add_fixit_insert (y, "."); + richloc.add_fixit_replace (colon, "="); + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + ASSERT_STREQ ("\n" + "FILENAME:3:24:\n" + " y\n" + " .\n" + "FILENAME:6:25:\n" + " : 0.0};\n" + " ^\n" + " =\n", + pp_formatted_text (dc.printer)); + } +} + + /* Verify that fix-it hints are appropriately consolidated. If any fix-it hints in a rich_location involve locations beyond @@ -1898,15 +2145,16 @@ test_fixit_consolidation (const line_table_case &case_) void diagnostic_show_locus_c_tests () { - test_range_contains_point_for_single_point (); - test_range_contains_point_for_single_line (); - test_range_contains_point_for_multiple_lines (); + test_layout_range_for_single_point (); + test_layout_range_for_single_line (); + test_layout_range_for_multiple_lines (); test_get_line_width_without_trailing_whitespace (); test_diagnostic_show_locus_unknown_location (); for_each_line_table_case (test_diagnostic_show_locus_one_liner); + for_each_line_table_case (test_diagnostic_show_locus_fixit_lines); for_each_line_table_case (test_fixit_consolidation); } diff --git a/libcpp/ChangeLog b/libcpp/ChangeLog index 595d6ca4cca..69abc6802bf 100644 --- a/libcpp/ChangeLog +++ b/libcpp/ChangeLog @@ -1,3 +1,12 @@ +2016-08-31 David Malcolm + + * include/line-map.h (class fixit_remove): Remove stray decl. + (fixit_hint::affects_line_p): Make const. + (fixit_insert::affects_line_p): Likewise. + (fixit_replace::affects_line_p): Likewise. + * line-map.c (fixit_insert::affects_line_p): Likewise. + (fixit_replace::affects_line_p): Likewise. + 2016-08-30 David Malcolm * include/line-map.h (class semi_embedded_vec): New class. diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index 0c95b292599..bef77957ffe 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -1412,7 +1412,6 @@ semi_embedded_vec::truncate (int len) class fixit_hint; class fixit_insert; - class fixit_remove; class fixit_replace; /* A "rich" source code location, for use when printing diagnostics. @@ -1599,7 +1598,7 @@ public: virtual ~fixit_hint () {} virtual enum kind get_kind () const = 0; - virtual bool affects_line_p (const char *file, int line) = 0; + virtual bool affects_line_p (const char *file, int line) const = 0; virtual source_location get_start_loc () const = 0; virtual bool maybe_get_end_loc (source_location *out) const = 0; /* Vfunc for consolidating successor fixits. */ @@ -1615,7 +1614,7 @@ class fixit_insert : public fixit_hint const char *new_content); ~fixit_insert (); enum kind get_kind () const { return INSERT; } - bool affects_line_p (const char *file, int line); + bool affects_line_p (const char *file, int line) const; source_location get_start_loc () const { return m_where; } bool maybe_get_end_loc (source_location *) const { return false; } bool maybe_append_replace (line_maps *set, @@ -1640,7 +1639,7 @@ class fixit_replace : public fixit_hint ~fixit_replace (); enum kind get_kind () const { return REPLACE; } - bool affects_line_p (const char *file, int line); + bool affects_line_p (const char *file, int line) const; source_location get_start_loc () const { return m_src_range.m_start; } bool maybe_get_end_loc (source_location *out) const { diff --git a/libcpp/line-map.c b/libcpp/line-map.c index 72549ba0732..f69c60c7837 100644 --- a/libcpp/line-map.c +++ b/libcpp/line-map.c @@ -2314,7 +2314,7 @@ fixit_insert::~fixit_insert () /* Implementation of fixit_hint::affects_line_p for fixit_insert. */ bool -fixit_insert::affects_line_p (const char *file, int line) +fixit_insert::affects_line_p (const char *file, int line) const { expanded_location exploc = linemap_client_expand_location_to_spelling_point (m_where); @@ -2351,7 +2351,7 @@ fixit_replace::~fixit_replace () /* Implementation of fixit_hint::affects_line_p for fixit_replace. */ bool -fixit_replace::affects_line_p (const char *file, int line) +fixit_replace::affects_line_p (const char *file, int line) const { return m_src_range.intersects_line_p (file, line); } -- 2.30.2