[committed] diagnostic-show-locus.c: handle fixits on lines outside the regular ranges

David Malcolm dmalcolm@redhat.com
Wed Aug 31 18:57:00 GMT 2016


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.

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

Committed to trunk as r239906.

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.
---
 gcc/diagnostic-show-locus.c | 292 ++++++++++++++++++++++++++++++++++++++++----
 libcpp/include/line-map.h   |   7 +-
 libcpp/line-map.c           |   4 +-
 3 files changed, 275 insertions(+), 28 deletions(-)

diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index a22a660..00a95a1 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 <layout_range> m_layout_ranges;
+  auto_vec <const fixit_hint *> m_fixit_hints;
   auto_vec <line_span> 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 <const fixit_insert *> (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 <const fixit_replace *> (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 <const fixit_insert *> (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 <const fixit_replace *> (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 <fixit_insert *> (hint);
+		const fixit_insert *insert
+		  = static_cast <const fixit_insert *> (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 <fixit_replace *> (hint);
+		const fixit_replace *replace
+		  = static_cast <const fixit_replace *> (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/include/line-map.h b/libcpp/include/line-map.h
index 0c95b29..bef7795 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1412,7 +1412,6 @@ semi_embedded_vec<T, NUM_EMBEDDED>::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 72549ba..f69c60c 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);
 }
-- 
1.8.5.3



More information about the Gcc-patches mailing list