[PATCH 02/02] C FE: add fix-it hint for . vs ->

[PATCH 02/02] C FE: add fix-it hint for . vs ->

[David Malcolm]

This is the most trivial example of a real fix-it example I could think
of: if the user writes
	ptr.field
rather than ptr->field.

gcc/c/ChangeLog:
	* c-typeck.c (build_component_ref): Special-case POINTER_TYPE when
	generating a "not a structure of union"  error message, and
	suggest a "->" rather than a ".", providing a fix-it hint.

gcc/testsuite/ChangeLog:
	* gcc.dg/fixits.c: New file.
---
 gcc/c/c-typeck.c              | 15 +++++++++++++++
 gcc/testsuite/gcc.dg/fixits.c | 14 ++++++++++++++
 2 files changed, 29 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/fixits.c

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index c2e16c6..6fe1ca8 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -2336,6 +2336,21 @@ build_component_ref (location_t loc, tree datum, tree component)
 
       return ref;
     }
+  else if (code == POINTER_TYPE && !c_dialect_objc ())
+    {
+      /* Special-case the error message for "ptr.field" for the case
+	 where the user has confused "." vs "->".
+	 We don't do it for Objective-C, since Objective-C 2.0 dot-syntax
+	 allows "." for ptrs; we could be handling a failed attempt
+	 to access a property.  */
+      rich_location richloc (line_table, loc);
+      /* "loc" should be the "." token.  */
+      richloc.add_fixit_replace (source_range::from_location (loc), "->");
+      error_at_rich_loc (&richloc,
+			 "%qE is a pointer; did you mean to use %<->%>?",
+			 datum);
+      return error_mark_node;
+    }
   else if (code != ERROR_MARK)
     error_at (loc,
 	      "request for member %qE in something not a structure or union",
diff --git a/gcc/testsuite/gcc.dg/fixits.c b/gcc/testsuite/gcc.dg/fixits.c
new file mode 100644
index 0000000..3b8c8a8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fixits.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-fdiagnostics-show-caret" } */
+
+struct foo { int x; };
+
+int test (struct foo *ptr)
+{
+  return ptr.x; /* { dg-error "'ptr' is a pointer; did you mean to use '->'?" } */
+/* { dg-begin-multiline-output "" }
+   return ptr.x;
+             ^
+             ->
+   { dg-end-multiline-output "" } */
+}
-- 
1.8.5.3

[PATCH 01/02] PR/62314: add ability to add fixit-hints

[David Malcolm]

This patch adds the ability to add "fix-it hints" to a rich_location,
which will be displayed when the corresponding diagnostic is printed.

It does not actually add any fix-it hints (that comes in the second
patch), but it adds test coverage of the machinery and printing,
by using the existing diagnostic_plugin_test_show_locus to inject
some meaningless fixit hints, and to verify the output.

For now, add a nasty linker kludge in layout::print_any_fixits for
the sake of diagnostic_plugin_test_show_locus.

Successfully bootstrapped&regrtested the pair of patches on
x86_64-pc-linux-gnu (on top of the 10-patch diagnostics kit).

OK for trunk?

gcc/ChangeLog:
	PR/62314
	* diagnostic-show-locus.c (colorizer::set_fixit_hint): New.
	(class layout): Update comment
	(layout::print_any_fixits): New method.
	(layout::move_to_column): New method.
	(diagnostic_show_locus): Add call to layout.print_any_fixits.

gcc/testsuite/ChangeLog:
	PR/62314
	* gcc.dg/plugin/diagnostic-test-show-locus-ascii-bw.c
	(test_fixit_insert): New.
	(test_fixit_remove): New.
	(test_fixit_replace): New.
	* gcc.dg/plugin/diagnostic-test-show-locus-ascii-color.c
	(test_fixit_insert): New.
	(test_fixit_remove): New.
	(test_fixit_replace): New.
	* gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
	(test_show_locus): Add tests of rendering fixit hints.

libcpp/ChangeLog:
	PR/62314
	* include/line-map.h (source_range::intersects_line_p): New
	method.
	(rich_location::~rich_location): New.
	(rich_location::add_fixit_insert): New method.
	(rich_location::add_fixit_remove): New method.
	(rich_location::add_fixit_replace): New method.
	(rich_location::get_num_fixit_hints): New accessor.
	(rich_location::get_fixit_hint): New accessor.
	(rich_location::MAX_FIXIT_HINTS): New constant.
	(rich_location::m_num_fixit_hints): New field.
	(rich_location::m_fixit_hints): New field.
	(class fixit_hint): New class.
	(class fixit_insert): New class.
	(class fixit_remove): New class.
	(class fixit_replace): New class.
	* line-map.c (source_range::intersects_line_p): New method.
	(rich_location::rich_location): Add initialization of
	m_num_fixit_hints to both ctors.
	(rich_location::~rich_location): New.
	(rich_location::add_fixit_insert): New method.
	(rich_location::add_fixit_remove): New method.
	(rich_location::add_fixit_replace): New method.
	(fixit_insert::fixit_insert): New.
	(fixit_insert::~fixit_insert): New.
	(fixit_insert::affects_line_p): New.
	(fixit_remove::fixit_remove): New.
	(fixit_remove::affects_line_p): New.
	(fixit_replace::fixit_replace): New.
	(fixit_replace::~fixit_replace): New.
	(fixit_replace::affects_line_p): New.
---
 gcc/diagnostic-show-locus.c                        | 125 ++++++++++++++++++-
 .../gcc.dg/plugin/diagnostic-test-show-locus-bw.c  |  43 +++++++
 .../plugin/diagnostic-test-show-locus-color.c      |  43 +++++++
 .../plugin/diagnostic_plugin_test_show_locus.c     |  35 ++++++
 libcpp/include/line-map.h                          |  96 +++++++++++++++
 libcpp/line-map.c                                  | 136 ++++++++++++++++++++-
 6 files changed, 471 insertions(+), 7 deletions(-)

diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 22203cd..f3d4a0e 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -78,6 +78,7 @@ class colorizer
 
   void set_range (int range_idx) { set_state (range_idx); }
   void set_normal_text () { set_state (STATE_NORMAL_TEXT); }
+  void set_fixit_hint () { set_state (0); }
 
  private:
   void set_state (int state);
@@ -139,8 +140,8 @@ struct line_bounds
 /* A class to control the overall layout when printing a diagnostic.
 
    The layout is determined within the constructor.
-   It is then printed by repeatedly calling the "print_source_line"
-   and "print_annotation_line" methods.
+   It is then printed by repeatedly calling the "print_source_line",
+   "print_annotation_line" and "print_any_fixits" methods.
 
    We assume we have disjoint ranges.  */
 
@@ -155,6 +156,7 @@ class layout
 
   bool print_source_line (int row, line_bounds *lbounds_out);
   void print_annotation_line (int row, const line_bounds lbounds);
+  void print_any_fixits (int row, const rich_location *richloc);
 
  private:
   bool
@@ -168,6 +170,9 @@ class layout
   get_x_bound_for_row (int row, int caret_column,
 		       int last_non_ws);
 
+  void
+  move_to_column (int *column, int dest_column);
+
  private:
   diagnostic_context *m_context;
   pretty_printer *m_pp;
@@ -593,6 +598,92 @@ layout::print_annotation_line (int row, const line_bounds lbounds)
   pp_newline (m_pp);
 }
 
+/* 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.  */
+
+void
+layout::print_any_fixits (int row, const rich_location *richloc)
+{
+  int column = 0;
+  for (unsigned int i = 0; i < richloc->get_num_fixit_hints (); i++)
+    {
+      fixit_hint *hint = richloc->get_fixit_hint (i);
+      if (hint->affects_line_p (m_exploc.file, row))
+	{
+	  /* For now we assume each fixit hint can only touch one line.  */
+	  switch (hint->get_kind ())
+	    {
+	    case fixit_hint::INSERT:
+	      {
+		fixit_insert *insert = static_cast <fixit_insert *> (hint);
+		/* This assumes the insertion just affects one line.  */
+		int start_column
+		  = LOCATION_COLUMN (insert->get_location ());
+		move_to_column (&column, start_column);
+		m_colorizer.set_fixit_hint ();
+		pp_string (m_pp, insert->get_string ());
+		m_colorizer.set_normal_text ();
+		column += insert->get_length ();
+	      }
+	      break;
+
+	    case fixit_hint::REMOVE:
+	      {
+		fixit_remove *remove = static_cast <fixit_remove *> (hint);
+		/* This assumes the removal just affects one line.  */
+		source_range src_range = remove->get_range ();
+		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++)
+		  {
+		    m_colorizer.set_fixit_hint ();
+		    pp_character (m_pp, '-');
+		    m_colorizer.set_normal_text ();
+		  }
+	      }
+	      break;
+
+	    case fixit_hint::REPLACE:
+	      {
+		fixit_replace *replace = static_cast <fixit_replace *> (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 ();
+	      }
+	      break;
+
+	    default:
+	      gcc_unreachable ();
+	    }
+	}
+    }
+
+  /* Nasty workaround to convince the linker to add
+      rich_location::add_fixit_insert
+      rich_location::add_fixit_remove
+      rich_location::add_fixit_replace
+     to cc1 for use by diagnostic_plugin_test_show_locus,
+     before anything in cc1 is using them.
+
+     This conditional should never hold, but hopefully the compiler can't
+     figure that out.  */
+  if ('.' == global_dc->caret_chars[0])
+    {
+      rich_location dummy (line_table, UNKNOWN_LOCATION);
+      dummy.add_fixit_insert (UNKNOWN_LOCATION, "");
+      dummy.add_fixit_remove
+	(source_range::from_location (UNKNOWN_LOCATION));
+      dummy.add_fixit_replace
+	(source_range::from_location (UNKNOWN_LOCATION), "");
+    }
+}
+
 /* Return true if (ROW/COLUMN) is within a range of the layout.
    If it returns true, OUT_STATE is written to, with the
    range index, and whether we should draw the caret at
@@ -675,6 +766,27 @@ layout::get_x_bound_for_row (int row, int caret_column,
   return result;
 }
 
+/* Given *COLUMN as an x-coordinate, print spaces to position
+   successive output at DEST_COLUMN, printing a newline if necessary,
+   and updating *COLUMN.  */
+
+void
+layout::move_to_column (int *column, int dest_column)
+{
+  /* Start a new line if we need to.  */
+  if (*column > dest_column)
+    {
+      pp_newline (m_pp);
+      *column = 0;
+    }
+
+  while (*column < dest_column)
+    {
+      pp_space (m_pp);
+      (*column)++;
+    }
+}
+
 } /* End of anonymous namespace.  */
 
 /* Print the physical source code corresponding to the location of
@@ -704,11 +816,14 @@ diagnostic_show_locus (diagnostic_context * context,
 	 row++)
       {
 	/* Print the source line, followed by an annotation line
-	   consisting of any caret/underlines.  If the source line can't
-	   be read, print nothing.  */
+	   consisting of any caret/underlines, then any fixits.
+	   If the source line can't be read, print nothing.  */
 	line_bounds lbounds;
 	if (layout.print_source_line (row, &lbounds))
-	  layout.print_annotation_line (row, lbounds);
+	  {
+	    layout.print_annotation_line (row, lbounds);
+	    layout.print_any_fixits (row, diagnostic->richloc);
+	  }
       }
 
     /* The closing scope here leads to the dtor for layout and thus
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c
index a4b16da..44b47e0 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c
@@ -147,3 +147,46 @@ void test_caret_on_leading_whitespace (void)
    { dg-end-multiline-output "" } */
 #endif
 }
+
+/* Unit test for rendering of insertion fixit hints
+   (example taken from PR 62316).  */
+
+void test_fixit_insert (void)
+{
+#if 0
+   int a[2][2] = { 0, 1 , 2, 3 }; /* { dg-warning "insertion hints" } */
+/* { dg-begin-multiline-output "" }
+    int a[2][2] = { 0, 1 , 2, 3 };
+                    ^~~~
+                    {   }
+   { dg-end-multiline-output "" } */
+#endif
+}
+
+/* Unit test for rendering of "remove" fixit hints.  */
+
+void test_fixit_remove (void)
+{
+#if 0
+  int a;; /* { dg-warning "example of a removal hint" } */
+/* { dg-begin-multiline-output "" }
+   int a;;
+         ^
+         -
+   { dg-end-multiline-output "" } */
+#endif
+}
+
+/* Unit test for rendering of "replace" fixit hints.  */
+
+void test_fixit_replace (void)
+{
+#if 0
+  gtk_widget_showall (dlg); /* { dg-warning "example of a replacement hint" } */
+/* { dg-begin-multiline-output "" }
+   gtk_widget_showall (dlg);
+   ^~~~~~~~~~~~~~~~~~
+   gtk_widget_show_all
+   { dg-end-multiline-output "" } */
+#endif
+}
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c
index 47639b2..199e0b2 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c
@@ -156,3 +156,46 @@ void test_caret_on_leading_whitespace (void)
    { dg-end-multiline-output "" } */
 #endif
 }
+
+/* Unit test for rendering of insertion fixit hints
+   (example taken from PR 62316).  */
+
+void test_fixit_insert (void)
+{
+#if 0
+   int a[2][2] = { 0, 1 , 2, 3 }; /* { dg-warning "insertion hints" } */
+/* { dg-begin-multiline-output "" }
+    int a[2][2] = { ^[[01;35m^[[K0, 1^[[m^[[K , 2, 3 };
+                    ^[[01;35m^[[K^~~~
+                    {^[[m^[[K   ^[[01;35m^[[K}^[[m^[[K
+   { dg-end-multiline-output "" } */
+#endif
+}
+
+/* Unit test for rendering of "remove" fixit hints.  */
+
+void test_fixit_remove (void)
+{
+#if 0
+  int a;; /* { dg-warning "example of a removal hint" } */
+/* { dg-begin-multiline-output "" }
+   int a;^[[01;35m^[[K;^[[m^[[K
+         ^[[01;35m^[[K^
+         -^[[m^[[K
+   { dg-end-multiline-output "" } */
+#endif
+}
+
+/* Unit test for rendering of "replace" fixit hints.  */
+
+void test_fixit_replace (void)
+{
+#if 0
+  gtk_widget_showall (dlg); /* { dg-warning "example of a replacement hint" } */
+/* { dg-begin-multiline-output "" }
+   ^[[01;35m^[[Kgtk_widget_showall^[[m^[[K (dlg);
+   ^[[01;35m^[[K^~~~~~~~~~~~~~~~~~
+   gtk_widget_show_all^[[m^[[K
+   { dg-end-multiline-output "" } */
+#endif
+}
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
index 158c612..7ff2cff 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
@@ -258,6 +258,41 @@ test_show_locus (function *fun)
       global_dc->caret_chars[1] = '^';
     }
 
+  /* Tests of rendering fixit hints.  */
+  if (0 == strcmp (fnname, "test_fixit_insert"))
+    {
+      const int line = fnstart_line + 2;
+      source_range src_range;
+      src_range.m_start = get_loc (line, 19);
+      src_range.m_finish = get_loc (line, 22);
+      rich_location richloc (src_range);
+      richloc.add_fixit_insert (src_range.m_start, "{");
+      richloc.add_fixit_insert (get_loc (line, 23), "}");
+      warning_at_rich_loc (&richloc, 0, "example of insertion hints");
+    }
+
+  if (0 == strcmp (fnname, "test_fixit_remove"))
+    {
+      const int line = fnstart_line + 2;
+      source_range src_range;
+      src_range.m_start = get_loc (line, 8);
+      src_range.m_finish = get_loc (line, 8);
+      rich_location richloc (src_range);
+      richloc.add_fixit_remove (src_range);
+      warning_at_rich_loc (&richloc, 0, "example of a removal hint");
+    }
+
+  if (0 == strcmp (fnname, "test_fixit_replace"))
+    {
+      const int line = fnstart_line + 2;
+      source_range src_range;
+      src_range.m_start = get_loc (line, 2);
+      src_range.m_finish = get_loc (line, 19);
+      rich_location richloc (src_range);
+      richloc.add_fixit_replace (src_range, "gtk_widget_show_all");
+      warning_at_rich_loc (&richloc, 0, "example of a replacement hint");
+    }
+
   /* Example of two carets where both carets appear to have an off-by-one
      error appearing one column early.
      Seen with gfortran.dg/associate_5.f03.
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index a7bc30c..36247b2 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -206,6 +206,9 @@ struct GTY(()) source_range
     result.m_finish = loc;
     return result;
   }
+
+  /* Is there any part of this range on the given line?  */
+  bool intersects_line_p (const char *file, int line) const;
 };
 
 /* Memory allocation function typedef.  Works like xrealloc.  */
@@ -1176,6 +1179,11 @@ struct location_range
   expanded_location m_caret;
 };
 
+class fixit_hint;
+  class fixit_insert;
+  class fixit_remove;
+  class fixit_replace;
+
 /* A "rich" source code location, for use when printing diagnostics.
    A rich_location has one or more ranges, each optionally with
    a caret.   Typically the zeroth range has a caret; other ranges
@@ -1258,6 +1266,9 @@ class rich_location
   /* Constructing from a source_range.  */
   rich_location (source_range src_range);
 
+  /* Destructor.  */
+  ~rich_location ();
+
   /* Accessors.  */
   source_location get_loc () const { return m_loc; }
 
@@ -1290,8 +1301,24 @@ class rich_location
   void
   override_column (int column);
 
+  /* Fix-it hints.  */
+  void
+  add_fixit_insert (source_location where,
+		    const char *new_content);
+
+  void
+  add_fixit_remove (source_range src_range);
+
+  void
+  add_fixit_replace (source_range src_range,
+		     const char *new_content);
+
+  unsigned int get_num_fixit_hints () const { return m_num_fixit_hints; }
+  fixit_hint *get_fixit_hint (int idx) const { return m_fixit_hints[idx]; }
+
 public:
   static const int MAX_RANGES = 3;
+  static const int MAX_FIXIT_HINTS = 2;
 
 protected:
   source_location m_loc;
@@ -1301,8 +1328,77 @@ protected:
 
   bool m_have_expanded_location;
   expanded_location m_expanded_location;
+
+  unsigned int m_num_fixit_hints;
+  fixit_hint *m_fixit_hints[MAX_FIXIT_HINTS];
+};
+
+class fixit_hint
+{
+public:
+  enum kind {INSERT, REMOVE, REPLACE};
+
+  virtual ~fixit_hint () {}
+
+  virtual enum kind get_kind () const = 0;
+  virtual bool affects_line_p (const char *file, int line) = 0;
+};
+
+class fixit_insert : public fixit_hint
+{
+ public:
+  fixit_insert (source_location where,
+		const char *new_content);
+  ~fixit_insert ();
+  enum kind get_kind () const { return INSERT; }
+  bool affects_line_p (const char *file, int line);
+
+  source_location get_location () const { return m_where; }
+  const char *get_string () const { return m_bytes; }
+  size_t get_length () const { return m_len; }
+
+ private:
+  source_location m_where;
+  char *m_bytes;
+  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_range get_range () const { return m_src_range; }
+
+ private:
+  source_range m_src_range;
 };
 
+class fixit_replace : public fixit_hint
+{
+ public:
+  fixit_replace (source_range src_range,
+                 const char *new_content);
+  ~fixit_replace ();
+
+  enum kind get_kind () const { return REPLACE; }
+  bool affects_line_p (const char *file, int line);
+
+  source_range get_range () const { return m_src_range; }
+  const char *get_string () const { return m_bytes; }
+  size_t get_length () const { return m_len; }
+
+ private:
+  source_range m_src_range;
+  char *m_bytes;
+  size_t m_len;
+};
+
+
 /* This is enum is used by the function linemap_resolve_location
    below.  The meaning of the values is explained in the comment of
    that function.  */
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index fe8d784..130abe8 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -1963,6 +1963,28 @@ line_table_dump (FILE *stream, struct line_maps *set, unsigned int num_ordinary,
     }
 }
 
+/* struct source_range.  */
+
+/* Is there any part of this range on the given line?  */
+
+bool
+source_range::intersects_line_p (const char *file, int line) const
+{
+  expanded_location exploc_start
+    = linemap_client_expand_location_to_spelling_point (m_start);
+  if (file != exploc_start.file)
+    return false;
+  if (line < exploc_start.line)
+      return false;
+  expanded_location exploc_finish
+    = linemap_client_expand_location_to_spelling_point (m_finish);
+  if (file != exploc_finish.file)
+    return false;
+  if (line > exploc_finish.line)
+      return false;
+  return true;
+}
+
 /* class rich_location.  */
 
 /* Construct a rich_location with location LOC as its initial range.  */
@@ -1970,7 +1992,8 @@ line_table_dump (FILE *stream, struct line_maps *set, unsigned int num_ordinary,
 rich_location::rich_location (line_maps *set, source_location loc) :
   m_loc (loc),
   m_num_ranges (0),
-  m_have_expanded_location (false)
+  m_have_expanded_location (false),
+  m_num_fixit_hints (0)
 {
   /* Set up the 0th range, extracting any range from LOC.  */
   source_range src_range = get_range_from_loc (set, loc);
@@ -1984,12 +2007,21 @@ rich_location::rich_location (line_maps *set, source_location loc) :
 rich_location::rich_location (source_range src_range)
 : m_loc (src_range.m_start),
   m_num_ranges (0),
-  m_have_expanded_location (false)
+  m_have_expanded_location (false),
+  m_num_fixit_hints (0)
 {
   /* Set up the 0th range: */
   add_range (src_range, true);
 }
 
+/* The destructor for class rich_location.  */
+
+rich_location::~rich_location ()
+{
+  for (unsigned int i = 0; i < m_num_fixit_hints; i++)
+    delete m_fixit_hints[i];
+}
+
 /* Get an expanded_location for this rich_location's primary
    location.  */
 
@@ -2093,3 +2125,103 @@ rich_location::set_range (unsigned int idx, source_range src_range,
       m_have_expanded_location = false;
     }
 }
+
+/* Add a fixit-hint, suggesting insertion of NEW_CONTENT
+   at WHERE.  */
+
+void
+rich_location::add_fixit_insert (source_location where,
+				 const char *new_content)
+{
+  linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
+  m_fixit_hints[m_num_fixit_hints++]
+    = new fixit_insert (where, new_content);
+}
+
+/* Add a fixit-hint, suggesting removal of the content at
+   SRC_RANGE.  */
+
+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 a fixit-hint, suggesting replacement of the content at
+   SRC_RANGE with NEW_CONTENT.  */
+
+void
+rich_location::add_fixit_replace (source_range src_range,
+				  const char *new_content)
+{
+  linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
+  m_fixit_hints[m_num_fixit_hints++]
+    = new fixit_replace (src_range, new_content);
+}
+
+/* class fixit_insert.  */
+
+fixit_insert::fixit_insert (source_location where,
+			    const char *new_content)
+: m_where (where),
+  m_bytes (xstrdup (new_content)),
+  m_len (strlen (new_content))
+{
+}
+
+fixit_insert::~fixit_insert ()
+{
+  free (m_bytes);
+}
+
+/* Implementation of fixit_hint::affects_line_p for fixit_insert.  */
+
+bool
+fixit_insert::affects_line_p (const char *file, int line)
+{
+  expanded_location exploc
+    = linemap_client_expand_location_to_spelling_point (m_where);
+  if (file == exploc.file)
+    if (line == exploc.line)
+      return true;
+  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,
+			      const char *new_content)
+: m_src_range (src_range),
+  m_bytes (xstrdup (new_content)),
+  m_len (strlen (new_content))
+{
+}
+
+fixit_replace::~fixit_replace ()
+{
+  free (m_bytes);
+}
+
+/* Implementation of fixit_hint::affects_line_p for fixit_replace.  */
+
+bool
+fixit_replace::affects_line_p (const char *file, int line)
+{
+  return m_src_range.intersects_line_p (file, line);
+}
-- 
1.8.5.3

Re: [PATCH 01/02] PR/62314: add ability to add fixit-hints

[Bernd Schmidt]

On 11/10/2015 05:35 PM, David Malcolm wrote:
> +  /* Nasty workaround to convince the linker to add
> +      rich_location::add_fixit_insert
> +      rich_location::add_fixit_remove
> +      rich_location::add_fixit_replace
> +     to cc1 for use by diagnostic_plugin_test_show_locus,
> +     before anything in cc1 is using them.
> +
> +     This conditional should never hold, but hopefully the compiler can't
> +     figure that out.  */

Does attribute((used)) help with this problem?


Bernd

Re: [PATCH 01/02] PR/62314: add ability to add fixit-hints

[David Malcolm]

On Tue, 2015-11-10 at 17:26 +0100, Bernd Schmidt wrote:
> On 11/10/2015 05:35 PM, David Malcolm wrote:
> > +  /* Nasty workaround to convince the linker to add
> > +      rich_location::add_fixit_insert
> > +      rich_location::add_fixit_remove
> > +      rich_location::add_fixit_replace
> > +     to cc1 for use by diagnostic_plugin_test_show_locus,
> > +     before anything in cc1 is using them.
> > +
> > +     This conditional should never hold, but hopefully the compiler can't
> > +     figure that out.  */
> 
> Does attribute((used)) help with this problem?

For some reason, I'm no longer seeing the problem; I tried simply taking
out the kludge, and it now works (this is *without* the in-cc1 usage in
patch 2); looking at cc1 shows that the above 3 symbols are indeed being
added:

$ eu-readelf -s ./cc1 |grep add_fixit
 2510: 00000000012a5280     94 FUNC    GLOBAL DEFAULT       13 _ZN13rich_location16add_fixit_insertEjPKc
 2905: 00000000012a5300     76 FUNC    GLOBAL DEFAULT       13 _ZN13rich_location16add_fixit_removeE12source_range
 9262: 00000000012a5390     94 FUNC    GLOBAL DEFAULT       13 _ZN13rich_location17add_fixit_replaceE12source_rangePKc
37430: 00000000012a5300     76 FUNC    GLOBAL DEFAULT       13 _ZN13rich_location16add_fixit_removeE12source_range
46935: 00000000012a5390     94 FUNC    GLOBAL DEFAULT       13 _ZN13rich_location17add_fixit_replaceE12source_rangePKc
47508: 00000000012a5280     94 FUNC    GLOBAL DEFAULT       13 _ZN13rich_location16add_fixit_insertEjPKc

I've tried poking at it, but I'm not sure what changed since I first
added the kludge (an earlier version of this, sent as:
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00732.html
); sorry.

Dave

Re: [PATCH 02/02] C FE: add fix-it hint for . vs ->

[Joseph Myers]

On Tue, 10 Nov 2015, David Malcolm wrote:

> This is the most trivial example of a real fix-it example I could think
> of: if the user writes
> 	ptr.field
> rather than ptr->field.
> 
> gcc/c/ChangeLog:
> 	* c-typeck.c (build_component_ref): Special-case POINTER_TYPE when
> 	generating a "not a structure of union"  error message, and
> 	suggest a "->" rather than a ".", providing a fix-it hint.

I wonder if this should be restricted to the case where the pointer's 
target is of structure or union type.  At least, if it's some other type, 
more of a fix is needed than just using -> (e.g. converting from void * to 
a pointer to the relevant type).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 02/02] C FE: add fix-it hint for . vs ->

[David Malcolm]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

On Tue, 2015-11-10 at 17:55 +0000, Joseph Myers wrote:
> On Tue, 10 Nov 2015, David Malcolm wrote:
> 
> > This is the most trivial example of a real fix-it example I could think
> > of: if the user writes
> > 	ptr.field
> > rather than ptr->field.
> > 
> > gcc/c/ChangeLog:
> > 	* c-typeck.c (build_component_ref): Special-case POINTER_TYPE when
> > 	generating a "not a structure of union"  error message, and
> > 	suggest a "->" rather than a ".", providing a fix-it hint.
> 
> I wonder if this should be restricted to the case where the pointer's 
> target is of structure or union type.  

Probably.  Attached is an updated version of the patch that does so.

> At least, if it's some other type, 
> more of a fix is needed than just using -> (e.g. converting from void * to 
> a pointer to the relevant type).

If so, then the attached patch simply does the status quo (I don't think
we want to try too hard for this case).  I've added a test case for
this.


[-- Attachment #2: 0001-C-FE-add-fix-it-hint-for-.-vs.patch --]
[-- Type: text/x-patch, Size: 4120 bytes --]

From 12aa183d693db59cbc5d8a268749d577e729425c Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Tue, 8 Sep 2015 12:56:00 -0400
Subject: [PATCH] C FE: add fix-it hint for . vs ->

This is the most trivial example of a real fix-it example I could think
of: if the user writes
	ptr.field
rather than ptr->field.

gcc/c/ChangeLog:
	* c-typeck.c (should_suggest_deref_p): New function.
	(build_component_ref): Special-case POINTER_TYPE when
	generating a "not a structure of union"  error message, and
	suggest a "->" rather than a ".", providing a fix-it hint.

gcc/testsuite/ChangeLog:
	* gcc.dg/fixits.c: New file.
---
 gcc/c/c-typeck.c              | 39 +++++++++++++++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/fixits.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/fixits.c

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index c2e16c6..bdb2d12 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -2249,6 +2249,33 @@ lookup_field (tree type, tree component)
   return tree_cons (NULL_TREE, field, NULL_TREE);
 }
 
+/* Support function for build_component_ref's error-handling.
+
+   Given DATUM_TYPE, and "DATUM.COMPONENT", where DATUM is *not* a
+   struct or union, should we suggest "DATUM->COMPONENT" as a hint?  */
+
+static bool
+should_suggest_deref_p (tree datum_type)
+{
+  /* We don't do it for Objective-C, since Objective-C 2.0 dot-syntax
+     allows "." for ptrs; we could be handling a failed attempt
+     to access a property.  */
+  if (c_dialect_objc ())
+    return false;
+
+  /* Only suggest it for pointers...  */
+  if (TREE_CODE (datum_type) != POINTER_TYPE)
+    return false;
+
+  /* ...to structs/unions.  */
+  tree underlying_type = TREE_TYPE (datum_type);
+  enum tree_code code = TREE_CODE (underlying_type);
+  if (code == RECORD_TYPE || code == UNION_TYPE)
+    return true;
+  else
+    return false;
+}
+
 /* Make an expression to refer to the COMPONENT field of structure or
    union value DATUM.  COMPONENT is an IDENTIFIER_NODE.  LOC is the
    location of the COMPONENT_REF.  */
@@ -2336,6 +2363,18 @@ build_component_ref (location_t loc, tree datum, tree component)
 
       return ref;
     }
+  else if (should_suggest_deref_p (type))
+    {
+      /* Special-case the error message for "ptr.field" for the case
+	 where the user has confused "." vs "->".  */
+      rich_location richloc (line_table, loc);
+      /* "loc" should be the "." token.  */
+      richloc.add_fixit_replace (source_range::from_location (loc), "->");
+      error_at_rich_loc (&richloc,
+			 "%qE is a pointer; did you mean to use %<->%>?",
+			 datum);
+      return error_mark_node;
+    }
   else if (code != ERROR_MARK)
     error_at (loc,
 	      "request for member %qE in something not a structure or union",
diff --git a/gcc/testsuite/gcc.dg/fixits.c b/gcc/testsuite/gcc.dg/fixits.c
new file mode 100644
index 0000000..06c9995
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fixits.c
@@ -0,0 +1,41 @@
+/* { dg-do compile } */
+/* { dg-options "-fdiagnostics-show-caret" } */
+
+struct foo { int x; };
+union u { int x; };
+
+/* Verify that we issue a hint for "." used with a ptr to a struct.  */
+
+int test_1 (struct foo *ptr)
+{
+  return ptr.x; /* { dg-error "'ptr' is a pointer; did you mean to use '->'?" } */
+/* { dg-begin-multiline-output "" }
+   return ptr.x;
+             ^
+             ->
+   { dg-end-multiline-output "" } */
+}
+
+/* Likewise for a ptr to a union.  */
+
+int test_2 (union u *ptr)
+{
+  return ptr.x; /* { dg-error "'ptr' is a pointer; did you mean to use '->'?" } */
+/* { dg-begin-multiline-output "" }
+   return ptr.x;
+             ^
+             ->
+   { dg-end-multiline-output "" } */
+}
+
+/* Verify that we don't issue a hint for a ptr to something that isn't a
+   struct or union.  */
+
+int test_3 (void **ptr)
+{
+  return ptr.x; /* { dg-error "request for member 'x' in something not a structure or union" } */
+/* { dg-begin-multiline-output "" }
+   return ptr.x;
+             ^
+   { dg-end-multiline-output "" } */
+}
-- 
1.8.5.3

Re: [PATCH 02/02] C FE: add fix-it hint for . vs ->

[Joseph Myers]

On Thu, 12 Nov 2015, David Malcolm wrote:

> On Tue, 2015-11-10 at 17:55 +0000, Joseph Myers wrote:
> > On Tue, 10 Nov 2015, David Malcolm wrote:
> > 
> > > This is the most trivial example of a real fix-it example I could think
> > > of: if the user writes
> > > 	ptr.field
> > > rather than ptr->field.
> > > 
> > > gcc/c/ChangeLog:
> > > 	* c-typeck.c (build_component_ref): Special-case POINTER_TYPE when
> > > 	generating a "not a structure of union"  error message, and
> > > 	suggest a "->" rather than a ".", providing a fix-it hint.
> > 
> > I wonder if this should be restricted to the case where the pointer's 
> > target is of structure or union type.  
> 
> Probably.  Attached is an updated version of the patch that does so.

This patch is OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 01/02] PR/62314: add ability to add fixit-hints

[Jeff Law]

On 11/10/2015 09:35 AM, David Malcolm wrote:
> This patch adds the ability to add "fix-it hints" to a rich_location,
> which will be displayed when the corresponding diagnostic is printed.
>
> It does not actually add any fix-it hints (that comes in the second
> patch), but it adds test coverage of the machinery and printing,
> by using the existing diagnostic_plugin_test_show_locus to inject
> some meaningless fixit hints, and to verify the output.
>
> For now, add a nasty linker kludge in layout::print_any_fixits for
> the sake of diagnostic_plugin_test_show_locus.
>
> Successfully bootstrapped&regrtested the pair of patches on
> x86_64-pc-linux-gnu (on top of the 10-patch diagnostics kit).
>
> OK for trunk?
>
> gcc/ChangeLog:
> 	PR/62314
> 	* diagnostic-show-locus.c (colorizer::set_fixit_hint): New.
> 	(class layout): Update comment
> 	(layout::print_any_fixits): New method.
> 	(layout::move_to_column): New method.
> 	(diagnostic_show_locus): Add call to layout.print_any_fixits.
>
> gcc/testsuite/ChangeLog:
> 	PR/62314
> 	* gcc.dg/plugin/diagnostic-test-show-locus-ascii-bw.c
> 	(test_fixit_insert): New.
> 	(test_fixit_remove): New.
> 	(test_fixit_replace): New.
> 	* gcc.dg/plugin/diagnostic-test-show-locus-ascii-color.c
> 	(test_fixit_insert): New.
> 	(test_fixit_remove): New.
> 	(test_fixit_replace): New.
> 	* gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
> 	(test_show_locus): Add tests of rendering fixit hints.
>
> libcpp/ChangeLog:
> 	PR/62314
> 	* include/line-map.h (source_range::intersects_line_p): New
> 	method.
> 	(rich_location::~rich_location): New.
> 	(rich_location::add_fixit_insert): New method.
> 	(rich_location::add_fixit_remove): New method.
> 	(rich_location::add_fixit_replace): New method.
> 	(rich_location::get_num_fixit_hints): New accessor.
> 	(rich_location::get_fixit_hint): New accessor.
> 	(rich_location::MAX_FIXIT_HINTS): New constant.
> 	(rich_location::m_num_fixit_hints): New field.
> 	(rich_location::m_fixit_hints): New field.
> 	(class fixit_hint): New class.
> 	(class fixit_insert): New class.
> 	(class fixit_remove): New class.
> 	(class fixit_replace): New class.
> 	* line-map.c (source_range::intersects_line_p): New method.
> 	(rich_location::rich_location): Add initialization of
> 	m_num_fixit_hints to both ctors.
> 	(rich_location::~rich_location): New.
> 	(rich_location::add_fixit_insert): New method.
> 	(rich_location::add_fixit_remove): New method.
> 	(rich_location::add_fixit_replace): New method.
> 	(fixit_insert::fixit_insert): New.
> 	(fixit_insert::~fixit_insert): New.
> 	(fixit_insert::affects_line_p): New.
> 	(fixit_remove::fixit_remove): New.
> 	(fixit_remove::affects_line_p): New.
> 	(fixit_replace::fixit_replace): New.
> 	(fixit_replace::~fixit_replace): New.
> 	(fixit_replace::affects_line_p): New.

> +
> +  /* Nasty workaround to convince the linker to add
> +      rich_location::add_fixit_insert
> +      rich_location::add_fixit_remove
> +      rich_location::add_fixit_replace
> +     to cc1 for use by diagnostic_plugin_test_show_locus,
> +     before anything in cc1 is using them.
> +
> +     This conditional should never hold, but hopefully the compiler can't
> +     figure that out.  */
> +  if ('.' == global_dc->caret_chars[0])
> +    {
> +      rich_location dummy (line_table, UNKNOWN_LOCATION);
> +      dummy.add_fixit_insert (UNKNOWN_LOCATION, "");
> +      dummy.add_fixit_remove
> +	(source_range::from_location (UNKNOWN_LOCATION));
> +      dummy.add_fixit_replace
> +	(source_range::from_location (UNKNOWN_LOCATION), "");
> +    }
> +}
So "the kludge" is presumably going to be removed based on later 
comments in this patch's thread.

What is the purpose of the #if 0 code in the various tests?  Did you 
mean to leave those in?


I think this is probably OK.  Though I am concerned that with blobs of 
the tests commented out that it's not being tested as thoroughly as you 
think it has :-)

Jeff

Re: [PATCH 01/02] PR/62314: add ability to add fixit-hints

[David Malcolm]

On Wed, 2015-11-18 at 14:57 -0700, Jeff Law wrote:
> On 11/10/2015 09:35 AM, David Malcolm wrote:
> > This patch adds the ability to add "fix-it hints" to a rich_location,
> > which will be displayed when the corresponding diagnostic is printed.
> >
> > It does not actually add any fix-it hints (that comes in the second
> > patch), but it adds test coverage of the machinery and printing,
> > by using the existing diagnostic_plugin_test_show_locus to inject
> > some meaningless fixit hints, and to verify the output.
> >
> > For now, add a nasty linker kludge in layout::print_any_fixits for
> > the sake of diagnostic_plugin_test_show_locus.
> >
> > Successfully bootstrapped&regrtested the pair of patches on
> > x86_64-pc-linux-gnu (on top of the 10-patch diagnostics kit).
> >
> > OK for trunk?
> >
> > gcc/ChangeLog:
> > 	PR/62314
> > 	* diagnostic-show-locus.c (colorizer::set_fixit_hint): New.
> > 	(class layout): Update comment
> > 	(layout::print_any_fixits): New method.
> > 	(layout::move_to_column): New method.
> > 	(diagnostic_show_locus): Add call to layout.print_any_fixits.
> >
> > gcc/testsuite/ChangeLog:
> > 	PR/62314
> > 	* gcc.dg/plugin/diagnostic-test-show-locus-ascii-bw.c
> > 	(test_fixit_insert): New.
> > 	(test_fixit_remove): New.
> > 	(test_fixit_replace): New.
> > 	* gcc.dg/plugin/diagnostic-test-show-locus-ascii-color.c
> > 	(test_fixit_insert): New.
> > 	(test_fixit_remove): New.
> > 	(test_fixit_replace): New.
> > 	* gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
> > 	(test_show_locus): Add tests of rendering fixit hints.
> >
> > libcpp/ChangeLog:
> > 	PR/62314
> > 	* include/line-map.h (source_range::intersects_line_p): New
> > 	method.
> > 	(rich_location::~rich_location): New.
> > 	(rich_location::add_fixit_insert): New method.
> > 	(rich_location::add_fixit_remove): New method.
> > 	(rich_location::add_fixit_replace): New method.
> > 	(rich_location::get_num_fixit_hints): New accessor.
> > 	(rich_location::get_fixit_hint): New accessor.
> > 	(rich_location::MAX_FIXIT_HINTS): New constant.
> > 	(rich_location::m_num_fixit_hints): New field.
> > 	(rich_location::m_fixit_hints): New field.
> > 	(class fixit_hint): New class.
> > 	(class fixit_insert): New class.
> > 	(class fixit_remove): New class.
> > 	(class fixit_replace): New class.
> > 	* line-map.c (source_range::intersects_line_p): New method.
> > 	(rich_location::rich_location): Add initialization of
> > 	m_num_fixit_hints to both ctors.
> > 	(rich_location::~rich_location): New.
> > 	(rich_location::add_fixit_insert): New method.
> > 	(rich_location::add_fixit_remove): New method.
> > 	(rich_location::add_fixit_replace): New method.
> > 	(fixit_insert::fixit_insert): New.
> > 	(fixit_insert::~fixit_insert): New.
> > 	(fixit_insert::affects_line_p): New.
> > 	(fixit_remove::fixit_remove): New.
> > 	(fixit_remove::affects_line_p): New.
> > 	(fixit_replace::fixit_replace): New.
> > 	(fixit_replace::~fixit_replace): New.
> > 	(fixit_replace::affects_line_p): New.
> 
> > +
> > +  /* Nasty workaround to convince the linker to add
> > +      rich_location::add_fixit_insert
> > +      rich_location::add_fixit_remove
> > +      rich_location::add_fixit_replace
> > +     to cc1 for use by diagnostic_plugin_test_show_locus,
> > +     before anything in cc1 is using them.
> > +
> > +     This conditional should never hold, but hopefully the compiler can't
> > +     figure that out.  */
> > +  if ('.' == global_dc->caret_chars[0])
> > +    {
> > +      rich_location dummy (line_table, UNKNOWN_LOCATION);
> > +      dummy.add_fixit_insert (UNKNOWN_LOCATION, "");
> > +      dummy.add_fixit_remove
> > +	(source_range::from_location (UNKNOWN_LOCATION));
> > +      dummy.add_fixit_replace
> > +	(source_range::from_location (UNKNOWN_LOCATION), "");
> > +    }
> > +}
> So "the kludge" is presumably going to be removed based on later 
> comments in this patch's thread.

Yes.

> What is the purpose of the #if 0 code in the various tests?  Did you 
> mean to leave those in?

Presumably you're referring to the bodies of the functions
  test_fixit_insert
  test_fixit_remove
  test_fixit_replace
within:
  gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c
  gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c

where the bodies are purely of the form:
{
  #if 0
  some code, containing dejagnu directives.
  #endif
}

Although this looks weird, it's deliberate, and follows the pattern
earlier in those test files: the diagnostics are injected by the plugin,
not by cc1.  The plugin gives us a way of unit-testing how
diagnostic_show_locus handles the various ways of calling into the
diagnostic API, isolating it from the details of any particular real
diagnostic in cc1, and from the details of how to get real
location/range information.

Hence we inject calls to the diagnostic API via the plugin, and the
bodies of the function could be anything - but I chose them to give
representative-looking diagnostics.  In fact, in
test_caret_on_leading_whitespace we have a fragment of Fortran code in a
C file (since this was a regression test for an issue I saw printing
Fortran diagnostics during development of the new
diagnostic_show_locus).

Hope this makes sense.  (FWIW there's a comment about this at the top of
diagnostic_plugin_test_show_locus.c, though of course that's not visible
in the patch under review).

Hence the testcase gives some examples of what uses of the 3 kinds of
fix-its might look like.  To actually implement those fix-its for those
diagnostics would be separate patches (fwiw I have some followups
already posted that update the levenshtein/spelling suggestion thing to
issue fix-its for the suggestions, but they'll have bit-rotted; I'll
update them and repost them).

(Ultimately we could have some kind of option to emit the fixit in
machine-parsable form, so that e.g. an IDE can offer to directly apply
the change; again, I see that as a followup).


> I think this is probably OK.  Though I am concerned that with blobs of 
> the tests commented out that it's not being tested as thoroughly as you 
> think it has :-)

Does the above address your concerns?  (Joesph already approved the
other patch)

Dave

Re: [PATCH 01/02] PR/62314: add ability to add fixit-hints

[Jeff Law]

On 11/18/2015 03:25 PM, David Malcolm wrote:

>
>> What is the purpose of the #if 0 code in the various tests?  Did you
>> mean to leave those in?
>
> Presumably you're referring to the bodies of the functions
>    test_fixit_insert
>    test_fixit_remove
>    test_fixit_replace
> within:
>    gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c
>    gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c
>
> where the bodies are purely of the form:
> {
>    #if 0
>    some code, containing dejagnu directives.
>    #endif
> }
Yes.  Those are the ones I'm referring to, I should have been more explicit.

>
> Although this looks weird, it's deliberate, and follows the pattern
> earlier in those test files: the diagnostics are injected by the plugin,
> not by cc1.  The plugin gives us a way of unit-testing how
> diagnostic_show_locus handles the various ways of calling into the
> diagnostic API, isolating it from the details of any particular real
> diagnostic in cc1, and from the details of how to get real
> location/range information.
*must* *remember* *this* *stuff* *is* *different*.

So essentially the code is there to mimick, to some degree, what we're 
going to warn for via the diagnostic API.  The code itself isn't of any 
real interest other than, essentially, documenting roughly what kidn of 
code we'd be warning about.

*must* *remember* *this* *stuff* *is* *different* :-)

>
> Does the above address your concerns?  (Joesph already approved the
> other patch)
Yes.  This is good to go into the trunk.

jeff

Re: [PATCH 01/02] PR/62314: add ability to add fixit-hints

[Bernd Schmidt]

On 11/24/2015 09:03 PM, Jeff Law wrote:
>>
>> Although this looks weird, it's deliberate, and follows the pattern
>> earlier in those test files: the diagnostics are injected by the plugin,
>> not by cc1.  The plugin gives us a way of unit-testing how
>> diagnostic_show_locus handles the various ways of calling into the
>> diagnostic API, isolating it from the details of any particular real
>> diagnostic in cc1, and from the details of how to get real
>> location/range information.
> *must* *remember* *this* *stuff* *is* *different*.

I think we should remember to fix that at some point not to use such a 
convoluted scheme.


Bernd