[PATCH] RFC: elide repeated source locations (PR other/84889)

[PATCH] RFC: elide repeated source locations (PR other/84889)

[David Malcolm]

We often emit more than one diagnostic at the same source location.
For example, the C++ frontend can emit many diagnostics at
the same source location when suggesting overload candidates.

For example:

../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In function 'int test_3(s, t)':
../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18: error: no match for 'operator&&' (operand types are 's' and 't')
   38 |   return param_s && param_t;
      |          ~~~~~~~~^~~~~~~~~~
../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18: note: candidate: 'operator&&(bool, bool)' <built-in>
../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18: note:   no known conversion for argument 2 from 't' to 'bool'

This is overly verbose.  Note how the same location has been printed
three times, obscuring the pertinent messages.

This patch add a new "elide" value to -fdiagnostics-show-location=
and makes it the default (previously it was "once").  With elision
the above is printed as:

../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In function 'int test_3(s, t)':
../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18: error: no match for 'operator&&' (operand types are 's' and 't')
   38 |   return param_s && param_t;
      |          ~~~~~~~~^~~~~~~~~~
      = note: candidate: 'operator&&(bool, bool)' <built-in>
      = note:   no known conversion for argument 2 from 't' to 'bool'

where the followup notes are printed with a '=' lined up with
the source code margin.

Thoughts?
Dave

TODO: the selftests currently assume LANG=C.  Clearly this would
need fixing before committing.

TODO: potentially integrate this with auto_diagnostic_group, so
we only elide within a logically-related diagnostic (and thus
make auto_diagnostic_group do something user-visible).

(FWIW, the output format is inspired by Rust)

gcc/ChangeLog:
	PR other/84889
	* common.opt (fdiagnostics-show-location=): Add "elide".
	(Enum(diagnostic_prefixing_rule)): Add "elide" value.
	* diagnostic-show-locus.c (layout::print_source_line): Set
	last_margin.
	(diagnostic_show_locus): Reset last_margin, saving it and
	restoring it if bailing out.
	(selftest::test_one_liner_elide_location): New test.
	FIXME: require LANG=C, or fix the test to work with i18n.
	(selftest::test_diagnostic_show_locus_one_liner): Call it.
	* diagnostic.c (diagnostic_initialize): Initialize "last_margin"
	and "override_file".
	(diagnostic_get_location_text): Use context->override_file
	if non-NULL.
	(diagnostic_build_prefix): Handle DIAGNOSTICS_SHOW_PREFIX_ELIDE.
	(selftest::assert_location_text): Set dc.override_file.
	* diagnostic.h (struct diagnostic_context): Add fields
	"last_margin" and "override_file".
	* doc/invoke.texi (-fdiagnostics-show-location=elide): Document.
	(-fdiagnostics-show-location=once): Remove "default behavior"
	text.
	* pretty-print.c (pp_set_real_maximum_length): Handle
	DIAGNOSTICS_SHOW_PREFIX_ELIDE.
	(pp_emit_prefix): Likewise.
	(pretty_printer::pretty_printer): Default to
	DIAGNOSTICS_SHOW_PREFIX_ELIDE.
	* pretty-print.h (enum diagnostic_prefixing_rule_t): Add
	DIAGNOSTICS_SHOW_PREFIX_ELIDE.  Document inline.
	* selftest-diagnostic.c
	(selftest::test_diagnostic_context::test_diagnostic_context):
	Set buffer's stream and flush_p.  Set override_file.
	(selftest::global_dc_test::global_dc_test): New ctor.
	(selftest::global_dc_test::~global_dc_test): New dtor.
	* selftest-diagnostic.h (class selftest::global_dc_test): New
	class.

gcc/testsuite/ChangeLog:
	PR other/84889
	* lib/prune.exp (TEST_ALWAYS_FLAGS): Add
	-fdiagnostics-show-location=once.
---
 gcc/common.opt              |   5 +-
 gcc/diagnostic-show-locus.c | 135 +++++++++++++++++++++++++++++++++++++++++++-
 gcc/diagnostic.c            |  70 ++++++++++++++++++++++-
 gcc/diagnostic.h            |   8 +++
 gcc/doc/invoke.texi         |  10 +++-
 gcc/pretty-print.c          |   7 ++-
 gcc/pretty-print.h          |  22 +++++---
 gcc/selftest-diagnostic.c   |  21 +++++++
 gcc/selftest-diagnostic.h   |  16 ++++++
 gcc/testsuite/lib/prune.exp |   1 +
 10 files changed, 279 insertions(+), 16 deletions(-)

diff --git a/gcc/common.opt b/gcc/common.opt
index 5a5d332..010ca0c 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1222,7 +1222,7 @@ Try to convert virtual calls to direct ones.
 
 fdiagnostics-show-location=
 Common Joined RejectNegative Enum(diagnostic_prefixing_rule)
--fdiagnostics-show-location=[once|every-line]	How often to emit source location at the beginning of line-wrapped diagnostics.
+-fdiagnostics-show-location=[elide|once|every-line]	How often to emit source location at the beginning of diagnostics.
 
 ; Required for these enum values.
 SourceInclude
@@ -1237,6 +1237,9 @@ Enum(diagnostic_prefixing_rule) String(once) Value(DIAGNOSTICS_SHOW_PREFIX_ONCE)
 EnumValue
 Enum(diagnostic_prefixing_rule) String(every-line) Value(DIAGNOSTICS_SHOW_PREFIX_EVERY_LINE)
 
+EnumValue
+Enum(diagnostic_prefixing_rule) String(elide) Value(DIAGNOSTICS_SHOW_PREFIX_ELIDE)
+
 fdiagnostics-show-caret
 Common Var(flag_diagnostics_show_caret) Init(1)
 Show the source line with a caret indicating the column.
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index a42ff81..2c8bb2b 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -1313,6 +1313,7 @@ layout::print_source_line (linenum_type row, const char *line, int line_width,
       for (int i = 0; i < m_linenum_width - width; i++)
 	pp_space (m_pp);
       pp_printf (m_pp, "%i | ", row);
+      m_context->last_margin = m_linenum_width + 1;
     }
   else
     pp_space (m_pp);
@@ -2309,6 +2310,11 @@ diagnostic_show_locus (diagnostic_context * context,
 {
   pp_newline (context->printer);
 
+  int saved_last_margin = context->last_margin;
+
+  /* Reset last_margin.  */
+  context->last_margin = -1;
+
   location_t loc = richloc->get_loc ();
   /* Do nothing if source-printing has been disabled.  */
   if (!context->show_caret)
@@ -2322,7 +2328,11 @@ diagnostic_show_locus (diagnostic_context * context,
      fix-it hints.  */
   if (loc == context->last_location
       && richloc->get_num_fixit_hints () == 0)
-    return;
+    {
+      /* Bail out, retaining the value of context->last_margin.  */
+      context->last_margin = saved_last_margin;
+      return;
+    }
 
   context->last_location = loc;
 
@@ -2841,6 +2851,128 @@ test_one_liner_labels ()
      gcc-rich-location.c due to Makefile.in issues).  */
 }
 
+/* Ensure that DIAGNOSTICS_SHOW_PREFIX_ELIDE works as expected.  */
+
+static void
+test_one_liner_elide_location ()
+{
+  location_t start = linemap_position_for_column (line_table, 11);
+  location_t finish = linemap_position_for_column (line_table, 15);
+  location_t field = make_location (start, start, finish);
+
+  /* Test the various combinations of
+     - with/without line numbers
+     - with/without elision.  */
+
+  /* With elision.  */
+  {
+    test_diagnostic_context dc;
+    global_dc_test gdt (&dc);
+    rich_location richloc (line_table, field);
+    richloc.add_fixit_replace ("m_field");
+    error_at (&richloc, "the error message");
+    inform (field, "the inform message");
+    ASSERT_STREQ ("FILENAME:1:11: error: the error message\n"
+		  " foo = bar.field;\n"
+		  "           ^~~~~\n"
+		  "           m_field\n"
+		  "   note: the inform message\n",
+		  pp_formatted_text (dc.printer));
+  }
+  {
+    test_diagnostic_context dc;
+    dc.show_line_numbers_p = true;
+    global_dc_test gdt (&dc);
+    rich_location richloc (line_table, field);
+    richloc.add_fixit_replace ("m_field");
+    error_at (&richloc, "the error message");
+    inform (field, "the 1st inform message");
+    inform (field, "the 2nd inform message");
+    ASSERT_STREQ ("FILENAME:1:11: error: the error message\n"
+		  "    1 | foo = bar.field;\n"
+		  "      |           ^~~~~\n"
+		  "      |           m_field\n"
+		  "      = note: the 1st inform message\n"
+		  "      = note: the 2nd inform message\n",
+		  pp_formatted_text (dc.printer));
+  }
+
+  {
+    /* Verify handling of UNKNOWN_LOCATION.  */
+    const char *old_progname = progname;
+    progname = "PROGNAME";
+    test_diagnostic_context dc;
+    dc.show_line_numbers_p = true;
+    global_dc_test gdt (&dc);
+    error_at (UNKNOWN_LOCATION, "the error message");
+    ASSERT_STREQ ("PROGNAME: error: the error message\n",
+		  pp_formatted_text (dc.printer));
+    progname = old_progname;
+  }
+  {
+    /* Verify handling of BUILTINS_LOCATION.  */
+    test_diagnostic_context dc;
+    dc.show_line_numbers_p = true;
+    global_dc_test gdt (&dc);
+    error_at (BUILTINS_LOCATION, "the error message");
+    inform (BUILTINS_LOCATION, "the inform message");
+    ASSERT_STREQ ("<built-in>: error: the error message\n"
+		  "<built-in>: note: the inform message\n",
+		  pp_formatted_text (dc.printer));
+  }
+  {
+    /* With a fix-it hint on the followup.  */
+    test_diagnostic_context dc;
+    dc.show_line_numbers_p = true;
+    global_dc_test gdt (&dc);
+    error_at (field, "the error message");
+    rich_location richloc (line_table, field);
+    richloc.add_fixit_replace ("m_field");
+    inform (&richloc, "the inform message");
+    ASSERT_STREQ ("FILENAME:1:11: error: the error message\n"
+		  "    1 | foo = bar.field;\n"
+		  "      |           ^~~~~\n"
+		  "      = note: the inform message\n"
+		  "    1 | foo = bar.field;\n"
+		  "      |           ^~~~~\n"
+		  "      |           m_field\n",
+		  pp_formatted_text (dc.printer));
+  }
+
+  /* Without elision.  */
+  {
+    test_diagnostic_context dc;
+    pp_prefixing_rule (dc.printer) = DIAGNOSTICS_SHOW_PREFIX_ONCE;
+    global_dc_test gdt (&dc);
+    rich_location richloc (line_table, field);
+    richloc.add_fixit_replace ("m_field");
+    error_at (&richloc, "the error message");
+    inform (field, "the inform message");
+    ASSERT_STREQ ("FILENAME:1:11: error: the error message\n"
+		  " foo = bar.field;\n"
+		  "           ^~~~~\n"
+		  "           m_field\n"
+		  "FILENAME:1:11: note: the inform message\n",
+		  pp_formatted_text (dc.printer));
+  }
+  {
+    test_diagnostic_context dc;
+    pp_prefixing_rule (dc.printer) = DIAGNOSTICS_SHOW_PREFIX_ONCE;
+    dc.show_line_numbers_p = true;
+    global_dc_test gdt (&dc);
+    rich_location richloc (line_table, field);
+    richloc.add_fixit_replace ("m_field");
+    error_at (&richloc, "the error message");
+    inform (field, "the inform message");
+    ASSERT_STREQ ("FILENAME:1:11: error: the error message\n"
+		  "    1 | foo = bar.field;\n"
+		  "      |           ^~~~~\n"
+		  "      |           m_field\n"
+		  "FILENAME:1:11: note: the inform message\n",
+		  pp_formatted_text (dc.printer));
+  }
+}
+
 /* Run the various one-liner tests.  */
 
 static void
@@ -2878,6 +3010,7 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_)
   test_one_liner_many_fixits_1 ();
   test_one_liner_many_fixits_2 ();
   test_one_liner_labels ();
+  test_one_liner_elide_location ();
 }
 
 /* Verify that gcc_rich_location::add_location_if_nearby works.  */
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index a572c08..72cbe9a 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -180,11 +180,13 @@ diagnostic_initialize (diagnostic_context *context, int n_opts)
   context->min_margin_width = 0;
   context->show_ruler_p = false;
   context->parseable_fixits_p = false;
+  context->last_margin = -1;
   context->edit_context_ptr = NULL;
   context->diagnostic_group_nesting_depth = 0;
   context->diagnostic_group_emission_count = 0;
   context->begin_group_cb = NULL;
   context->end_group_cb = NULL;
+  context->override_file = NULL;
 }
 
 /* Maybe initialize the color support. We require clients to do this
@@ -327,8 +329,25 @@ diagnostic_get_location_text (diagnostic_context *context,
   pretty_printer *pp = context->printer;
   const char *locus_cs = colorize_start (pp_show_color (pp), "locus");
   const char *locus_ce = colorize_stop (pp_show_color (pp));
-  const char *file = s.file ? s.file : progname;
-  int line = strcmp (file, N_("<built-in>")) ? s.line : 0;
+  const char *file;
+  int line;
+  if (s.file)
+    {
+      file = s.file;
+      if (strcmp (file, N_("<built-in>")))
+	{
+	  if (context->override_file)
+	    file = context->override_file;
+	  line = s.line;
+	}
+      else
+	line = 0;
+    }
+  else
+    {
+      file = progname;
+      line = s.line;
+    }
   int col = context->show_column ? s.column : 0;
 
   const char *line_col = maybe_line_and_column (line, col);
@@ -362,6 +381,52 @@ diagnostic_build_prefix (diagnostic_context *context,
       text_ce = colorize_stop (pp_show_color (pp));
     }
 
+  /* Don't bother printing the location in the prefix if we have a diagnostic
+     with the same location as the previous diagnostic.  This makes the
+     diagnostics less "dense" visually, so that e.g.:
+
+       PATH_WHICH_COULD_BE_VERY_LONG/SOME_FILENAME:LINE:COLUMN: error: MESSAGE_1
+        SOURCE VIEW
+       PATH_WHICH_COULD_BE_VERY_LONG/SOME_FILENAME:LINE:COLUMN: note: MESSAGE_2
+       PATH_WHICH_COULD_BE_VERY_LONG/SOME_FILENAME:LINE:COLUMN: note: MESSAGE_3
+
+     can be instead printed as:
+
+       PATH_WHICH_COULD_BE_VERY_LONG/SOME_FILENAME:LINE:COLUMN: error: MESSAGE_1
+        SOURCE VIEW
+          note: MESSAGE_2
+          note: MESSAGE_3
+
+     or, with line numbers:
+
+       PATH_WHICH_COULD_BE_VERY_LONG/SOME_FILENAME:LINE:COLUMN: error: MESSAGE_1
+       nn | SOURCE VIEW
+          = note: MESSAGE_2
+          = note: MESSAGE_3
+
+     This should also make it easier for the user to see the boundaries between
+     logically-related diagnostics.  */
+  if (pp_prefixing_rule (pp) == DIAGNOSTICS_SHOW_PREFIX_ELIDE)
+    {
+      location_t loc = diagnostic->richloc->get_loc ();
+      // FIXME: but not if a builtin location?
+      if (loc == context->last_location
+	  && loc != UNKNOWN_LOCATION)
+	{
+	  /* last_margin is set when printing the left margin.  */
+	  if (context->last_margin >= 0)
+	    {
+	      pretty_printer pp;
+	      for (int i = 0; i < context->last_margin; i++)
+		pp_character (&pp, ' ');
+	      pp_printf (&pp, "= %s%s%s", text_cs, text, text_ce);
+	      return xstrdup (pp_formatted_text (&pp));
+	    }
+	  else
+	    return build_message_string ("   %s%s%s", text_cs, text, text_ce);
+	}
+    }
+
   expanded_location s = diagnostic_expand_location (diagnostic);
   char *location_text = diagnostic_get_location_text (context, s);
 
@@ -1740,6 +1805,7 @@ assert_location_text (const char *expected_loc_text,
 {
   test_diagnostic_context dc;
   dc.show_column = show_column;
+  dc.override_file = NULL;
 
   expanded_location xloc;
   xloc.file = filename;
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 3498a9b..28ac278 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -223,6 +223,10 @@ struct diagnostic_context
      rest of the diagnostic.  */
   bool parseable_fixits_p;
 
+  /* For use with DIAGNOSTICS_SHOW_PREFIX_ELIDE: the width of the last
+     margin, or -1 if there was no margin.  */
+  int last_margin;
+
   /* If non-NULL, an edit_context to which fix-it hints should be
      applied, for generating patches.  */
   edit_context *edit_context_ptr;
@@ -243,6 +247,10 @@ struct diagnostic_context
   /* If non-NULL, this will be called when a stack of groups is
      popped if any diagnostics were emitted within that group.  */
   void (*end_group_cb) (diagnostic_context * context);
+
+  /* For use in selftests: if non-NULL, then use this string when
+     printing filenames.  */
+  const char *override_file;
 };
 
 static inline void
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 4ff3a150..49b7962 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -3673,14 +3673,20 @@ Note - this option also affects the display of the @samp{#error} and
 function/type/variable attribute.  It does not however affect the
 @samp{pragma GCC warning} and @samp{pragma GCC error} pragmas.
 
+@item -fdiagnostics-show-location=elide
+@opindex fdiagnostics-show-location
+Instructs the diagnostic messages reporter to only emit source location
+information once within a given diagnostic, and to elide such locations
+for followup diagnostics that have the same location.
+This is the default behavior.
+
 @item -fdiagnostics-show-location=once
 @opindex fdiagnostics-show-location
 Only meaningful in line-wrapping mode.  Instructs the diagnostic messages
 reporter to emit source location information @emph{once}; that is, in
 case the message is too long to fit on a single physical line and has to
 be wrapped, the source location won't be emitted (as prefix) again,
-over and over, in subsequent continuation lines.  This is the default
-behavior.
+over and over, in subsequent continuation lines.
 
 @item -fdiagnostics-show-location=every-line
 Only meaningful in line-wrapping mode.  Instructs the diagnostic
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index 691dbb6..a168207 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -783,6 +783,7 @@ pp_set_real_maximum_length (pretty_printer *pp)
      not to increase unnecessarily the line-length cut-off.  */
   if (!pp_is_wrapping_line (pp)
       || pp_prefixing_rule (pp) == DIAGNOSTICS_SHOW_PREFIX_ONCE
+      || pp_prefixing_rule (pp) == DIAGNOSTICS_SHOW_PREFIX_ELIDE
       || pp_prefixing_rule (pp) == DIAGNOSTICS_SHOW_PREFIX_NEVER)
     pp->maximum_length = pp_line_cutoff (pp);
   else
@@ -1546,6 +1547,7 @@ pp_emit_prefix (pretty_printer *pp)
 	  break;
 
 	case DIAGNOSTICS_SHOW_PREFIX_ONCE:
+	case DIAGNOSTICS_SHOW_PREFIX_ELIDE:
 	  if (pp->emitted_prefix)
 	    {
 	      pp_indent (pp);
@@ -1582,8 +1584,9 @@ pretty_printer::pretty_printer (int maximum_length)
     show_color ()
 {
   pp_line_cutoff (this) = maximum_length;
-  /* By default, we emit prefixes once per message.  */
-  pp_prefixing_rule (this) = DIAGNOSTICS_SHOW_PREFIX_ONCE;
+  /* Emit only once within a given diagnostic, and elide locations
+     for followup diagnostics with equal location.  */
+  pp_prefixing_rule (this) = DIAGNOSTICS_SHOW_PREFIX_ELIDE;
   pp_set_prefix (this, NULL);
 }
 
diff --git a/gcc/pretty-print.h b/gcc/pretty-print.h
index a6e60f1..316c611 100644
--- a/gcc/pretty-print.h
+++ b/gcc/pretty-print.h
@@ -41,16 +41,22 @@ struct text_info
   location_t get_location (unsigned int index_of_location) const;
 };
 
-/* How often diagnostics are prefixed by their locations:
-   o DIAGNOSTICS_SHOW_PREFIX_NEVER: never - not yet supported;
-   o DIAGNOSTICS_SHOW_PREFIX_ONCE: emit only once;
-   o DIAGNOSTICS_SHOW_PREFIX_EVERY_LINE: emit each time a physical
-   line is started.  */
+/* How often diagnostics are prefixed by their locations.  */
 enum diagnostic_prefixing_rule_t
 {
-  DIAGNOSTICS_SHOW_PREFIX_ONCE       = 0x0,
-  DIAGNOSTICS_SHOW_PREFIX_NEVER      = 0x1,
-  DIAGNOSTICS_SHOW_PREFIX_EVERY_LINE = 0x2
+  /* Emit only once within a given diagnostic.  */
+  DIAGNOSTICS_SHOW_PREFIX_ONCE,
+
+  /* Emit only once within a given diagnostic, and elide locations
+     for followup diagnostics with equal location.  */
+  DIAGNOSTICS_SHOW_PREFIX_ELIDE,
+
+  /* Never - not yet supported.  */
+  DIAGNOSTICS_SHOW_PREFIX_NEVER,
+
+  /* Emit each time a physical line is started within a diagnostic
+     message.  */
+  DIAGNOSTICS_SHOW_PREFIX_EVERY_LINE
 };
 
 /* The chunk_info data structure forms a stack of the results from the
diff --git a/gcc/selftest-diagnostic.c b/gcc/selftest-diagnostic.c
index 4a7f0de..4e6cfd4 100644
--- a/gcc/selftest-diagnostic.c
+++ b/gcc/selftest-diagnostic.c
@@ -36,11 +36,15 @@ namespace selftest {
 test_diagnostic_context::test_diagnostic_context ()
 {
   diagnostic_initialize (this, 0);
+  /* Don't print to stderr.  */
+  printer->buffer->stream = NULL;
+  printer->buffer->flush_p = false;
   show_caret = true;
   show_labels_p = true;
   show_column = true;
   start_span = start_span_cb;
   min_margin_width = 6;
+  override_file = "FILENAME";
 }
 
 test_diagnostic_context::~test_diagnostic_context ()
@@ -59,6 +63,23 @@ test_diagnostic_context::start_span_cb (diagnostic_context *context,
   default_diagnostic_start_span_fn (context, exploc);
 }
 
+/* Implementation of class selftest::global_dc_test.  */
+
+/* Constructor.  Override "global_dc" with DC.  */
+
+global_dc_test::global_dc_test (diagnostic_context *dc)
+: m_saved_global_dc (global_dc)
+{
+  global_dc = dc;
+}
+
+/* Destructor.  Restore the saved global_dc.  */
+
+global_dc_test::~global_dc_test ()
+{
+  global_dc = m_saved_global_dc;
+}
+
 } // namespace selftest
 
 #endif /* #if CHECKING_P */
diff --git a/gcc/selftest-diagnostic.h b/gcc/selftest-diagnostic.h
index b1d6ace..90ff2d1 100644
--- a/gcc/selftest-diagnostic.h
+++ b/gcc/selftest-diagnostic.h
@@ -42,6 +42,22 @@ class test_diagnostic_context : public diagnostic_context
   start_span_cb (diagnostic_context *context, expanded_location exploc);
 };
 
+/* A class for overriding the global "global_dc" within a selftest,
+   restoring its value afterwards.  */
+
+class global_dc_test
+{
+ public:
+  /* Constructor.  Override "global_dc" with DC.  */
+  global_dc_test (diagnostic_context *dc);
+
+  /* Destructor.  Restore the saved global_dc.  */
+  ~global_dc_test ();
+
+ private:
+  diagnostic_context *m_saved_global_dc;
+};
+
 } // namespace selftest
 
 #endif /* #if CHECKING_P */
diff --git a/gcc/testsuite/lib/prune.exp b/gcc/testsuite/lib/prune.exp
index df36c34..9e08545 100644
--- a/gcc/testsuite/lib/prune.exp
+++ b/gcc/testsuite/lib/prune.exp
@@ -22,6 +22,7 @@ if ![info exists TEST_ALWAYS_FLAGS] {
     set TEST_ALWAYS_FLAGS ""
 }
 set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never $TEST_ALWAYS_FLAGS"
+set TEST_ALWAYS_FLAGS "-fdiagnostics-show-location=once $TEST_ALWAYS_FLAGS"
 
 proc prune_gcc_output { text } {
     global srcdir
-- 
1.8.5.3

Re: [PATCH] RFC: elide repeated source locations (PR other/84889)

[Martin Sebor]

On 11/11/2018 07:43 PM, David Malcolm wrote:
> We often emit more than one diagnostic at the same source location.
> For example, the C++ frontend can emit many diagnostics at
> the same source location when suggesting overload candidates.
>
> For example:
>
> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In function 'int test_3(s, t)':
> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18: error: no match for 'operator&&' (operand types are 's' and 't')
>    38 |   return param_s && param_t;
>       |          ~~~~~~~~^~~~~~~~~~
> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18: note: candidate: 'operator&&(bool, bool)' <built-in>
> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18: note:   no known conversion for argument 2 from 't' to 'bool'
>
> This is overly verbose.  Note how the same location has been printed
> three times, obscuring the pertinent messages.
>
> This patch add a new "elide" value to -fdiagnostics-show-location=
> and makes it the default (previously it was "once").  With elision
> the above is printed as:
>
> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In function 'int test_3(s, t)':
> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18: error: no match for 'operator&&' (operand types are 's' and 't')
>    38 |   return param_s && param_t;
>       |          ~~~~~~~~^~~~~~~~~~
>       = note: candidate: 'operator&&(bool, bool)' <built-in>
>       = note:   no known conversion for argument 2 from 't' to 'bool'
>
> where the followup notes are printed with a '=' lined up with
> the source code margin.
>
> Thoughts?

I agree the long pathname in the notes is at first glance redundant
but I'm not sure about using '=' as a shorthand for it.  I have
written many scripts to parse GCC output to extract all diagnostics
(including notes) and publish those on a Web page somewhere, as I'm
sure must have others.  All those scripts would stop working with
this change and require changes to the build system to work again.
Making those changes can be a substantial undertaking in some
organizations.

Have you considered printing just the file name instead?  Or any
other alternatives?

Martin

Re: [PATCH] RFC: elide repeated source locations (PR other/84889)

[David Malcolm]

On Mon, 2018-11-12 at 13:37 -0700, Martin Sebor wrote:
> On 11/11/2018 07:43 PM, David Malcolm wrote:
> > We often emit more than one diagnostic at the same source location.
> > For example, the C++ frontend can emit many diagnostics at
> > the same source location when suggesting overload candidates.
> > 
> > For example:
> > 
> > ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In
> > function 'int test_3(s, t)':
> > ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
> > error: no match for 'operator&&' (operand types are 's' and 't')
> >    38 |   return param_s && param_t;
> >       |          ~~~~~~~~^~~~~~~~~~
> > ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
> > note: candidate: 'operator&&(bool, bool)' <built-in>
> > ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
> > note:   no known conversion for argument 2 from 't' to 'bool'
> > 
> > This is overly verbose.  Note how the same location has been
> > printed
> > three times, obscuring the pertinent messages.
> > 
> > This patch add a new "elide" value to -fdiagnostics-show-location=
> > and makes it the default (previously it was "once").  With elision
> > the above is printed as:
> > 
> > ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In
> > function 'int test_3(s, t)':
> > ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
> > error: no match for 'operator&&' (operand types are 's' and 't')
> >    38 |   return param_s && param_t;
> >       |          ~~~~~~~~^~~~~~~~~~
> >       = note: candidate: 'operator&&(bool, bool)' <built-in>
> >       = note:   no known conversion for argument 2 from 't' to
> > 'bool'
> > 
> > where the followup notes are printed with a '=' lined up with
> > the source code margin.
> > 
> > Thoughts?
> 
> I agree the long pathname in the notes is at first glance redundant
> but I'm not sure about using '=' as a shorthand for it.  I have
> written many scripts to parse GCC output to extract all diagnostics
> (including notes) and publish those on a Web page somewhere, as I'm
> sure must have others.  All those scripts would stop working with
> this change and require changes to the build system to work again.
> Making those changes can be a substantial undertaking in some
> organizations.
> 
> Have you considered printing just the file name instead?  Or any
> other alternatives?

"-fdiagnostics-show-location=once" will restore the old behavior.
Alternatively, if you want to parse GCC output, I'm adding a JSON
output format; see:
  https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01038.html
(I'm testing an updated version of that patch)

Dave

Re: [PATCH] RFC: elide repeated source locations (PR other/84889)

[Martin Sebor]

On 11/14/2018 02:12 PM, David Malcolm wrote:
> On Mon, 2018-11-12 at 13:37 -0700, Martin Sebor wrote:
>> On 11/11/2018 07:43 PM, David Malcolm wrote:
>>> We often emit more than one diagnostic at the same source location.
>>> For example, the C++ frontend can emit many diagnostics at
>>> the same source location when suggesting overload candidates.
>>>
>>> For example:
>>>
>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In
>>> function 'int test_3(s, t)':
>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>> error: no match for 'operator&&' (operand types are 's' and 't')
>>>    38 |   return param_s && param_t;
>>>       |          ~~~~~~~~^~~~~~~~~~
>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>> note: candidate: 'operator&&(bool, bool)' <built-in>
>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>> note:   no known conversion for argument 2 from 't' to 'bool'
>>>
>>> This is overly verbose.  Note how the same location has been
>>> printed
>>> three times, obscuring the pertinent messages.
>>>
>>> This patch add a new "elide" value to -fdiagnostics-show-location=
>>> and makes it the default (previously it was "once").  With elision
>>> the above is printed as:
>>>
>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In
>>> function 'int test_3(s, t)':
>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>> error: no match for 'operator&&' (operand types are 's' and 't')
>>>    38 |   return param_s && param_t;
>>>       |          ~~~~~~~~^~~~~~~~~~
>>>       = note: candidate: 'operator&&(bool, bool)' <built-in>
>>>       = note:   no known conversion for argument 2 from 't' to
>>> 'bool'
>>>
>>> where the followup notes are printed with a '=' lined up with
>>> the source code margin.
>>>
>>> Thoughts?
>>
>> I agree the long pathname in the notes is at first glance redundant
>> but I'm not sure about using '=' as a shorthand for it.  I have
>> written many scripts to parse GCC output to extract all diagnostics
>> (including notes) and publish those on a Web page somewhere, as I'm
>> sure must have others.  All those scripts would stop working with
>> this change and require changes to the build system to work again.
>> Making those changes can be a substantial undertaking in some
>> organizations.
>>
>> Have you considered printing just the file name instead?  Or any
>> other alternatives?
>
> "-fdiagnostics-show-location=once" will restore the old behavior.
> Alternatively, if you want to parse GCC output, I'm adding a JSON
> output format; see:
>   https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01038.html
> (I'm testing an updated version of that patch)

I understand the change can be disabled by an option.  I also
like making the repetitive pathnames shorter, but the concern
I have is that the change to use the '=' notation by default,
besides being rather unusual and not necessarily intuitive, will
break scripts that search for the pattern:

   "[^:]*:[1-9][0-9]*:[1-9][0-9]*: (error|note|warning): "

and adding a new option to a large build system can be quite
difficult.  I suspect it would also make the notes difficult
or even impossible to associate with the corresponding errors
or warnings in parallel builds (where they may be interleaved
with diagnostics from different compilations).

I think it's possible to make the output shorter/less repetitive
without these side-effects by keeping the current format and
instead abbreviating the pathname, e.g. by printing just the file
name (or ".../filename.c:" with the ellipsis standing in for the
long pathname, though the ellipsis would require adjusting
the scripts that sort diagnostics by the pathname).

Martin

PS As an aside, if we wanted to shorten all pathnames this same
idea could be extended to errors and warnings as well by printing
the pathname in the first one and just the filename in subsequent
ones in the same file.

Re: [PATCH] RFC: elide repeated source locations (PR other/84889)

[Eric Gallager]

On 11/15/18, Martin Sebor <msebor@gmail.com> wrote:
> On 11/14/2018 02:12 PM, David Malcolm wrote:
>> On Mon, 2018-11-12 at 13:37 -0700, Martin Sebor wrote:
>>> On 11/11/2018 07:43 PM, David Malcolm wrote:
>>>> We often emit more than one diagnostic at the same source location.
>>>> For example, the C++ frontend can emit many diagnostics at
>>>> the same source location when suggesting overload candidates.
>>>>
>>>> For example:
>>>>
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In
>>>> function 'int test_3(s, t)':
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>>> error: no match for 'operator&&' (operand types are 's' and 't')
>>>>    38 |   return param_s && param_t;
>>>>       |          ~~~~~~~~^~~~~~~~~~
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>>> note: candidate: 'operator&&(bool, bool)' <built-in>
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>>> note:   no known conversion for argument 2 from 't' to 'bool'
>>>>
>>>> This is overly verbose.  Note how the same location has been
>>>> printed
>>>> three times, obscuring the pertinent messages.
>>>>
>>>> This patch add a new "elide" value to -fdiagnostics-show-location=
>>>> and makes it the default (previously it was "once").  With elision
>>>> the above is printed as:
>>>>
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In
>>>> function 'int test_3(s, t)':
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>>> error: no match for 'operator&&' (operand types are 's' and 't')
>>>>    38 |   return param_s && param_t;
>>>>       |          ~~~~~~~~^~~~~~~~~~
>>>>       = note: candidate: 'operator&&(bool, bool)' <built-in>
>>>>       = note:   no known conversion for argument 2 from 't' to
>>>> 'bool'
>>>>
>>>> where the followup notes are printed with a '=' lined up with
>>>> the source code margin.
>>>>
>>>> Thoughts?
>>>
>>> I agree the long pathname in the notes is at first glance redundant
>>> but I'm not sure about using '=' as a shorthand for it.

I like the -fdiagnostics-show-location=elide option but I also agree
about the '=' looking weird. However, I don't think the '=' is that
big a problem that needs to provoke huge rethinking, just some minor
bikeshedding about which symbol to use instead. How about '+'?

>>> I have
>>> written many scripts to parse GCC output to extract all diagnostics
>>> (including notes) and publish those on a Web page somewhere, as I'm
>>> sure must have others.  All those scripts would stop working with
>>> this change and require changes to the build system to work again.
>>> Making those changes can be a substantial undertaking in some
>>> organizations.
>>>
>>> Have you considered printing just the file name instead?  Or any
>>> other alternatives?
>>
>> "-fdiagnostics-show-location=once" will restore the old behavior.
>> Alternatively, if you want to parse GCC output, I'm adding a JSON
>> output format; see:
>>   https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01038.html
>> (I'm testing an updated version of that patch)
>
> I understand the change can be disabled by an option.  I also
> like making the repetitive pathnames shorter, but the concern
> I have is that the change to use the '=' notation by default,
> besides being rather unusual and not necessarily intuitive, will
> break scripts that search for the pattern:
>
>    "[^:]*:[1-9][0-9]*:[1-9][0-9]*: (error|note|warning): "
>
> and adding a new option to a large build system can be quite
> difficult.  I suspect it would also make the notes difficult
> or even impossible to associate with the corresponding errors
> or warnings in parallel builds (where they may be interleaved
> with diagnostics from different compilations).
>
> I think it's possible to make the output shorter/less repetitive
> without these side-effects by keeping the current format and
> instead abbreviating the pathname, e.g. by printing just the file
> name (or ".../filename.c:" with the ellipsis standing in for the
> long pathname, though the ellipsis would require adjusting
> the scripts that sort diagnostics by the pathname).
>
> Martin
>
> PS As an aside, if we wanted to shorten all pathnames this same
> idea could be extended to errors and warnings as well by printing
> the pathname in the first one and just the filename in subsequent
> ones in the same file.
>