PR fortran/91426: Colorize %L text to match diagnostic_show_locus
diff mbox series

Message ID 20190920204547.13300-1-dmalcolm@redhat.com
State New
Headers show
Series
  • PR fortran/91426: Colorize %L text to match diagnostic_show_locus
Related show

Commit Message

David Malcolm Sept. 20, 2019, 8:45 p.m. UTC
PR fortran/91426 reports that the following program

program main
10 continue
10 continue
end

yields:

label.f90:2:2:

    2 | 10 continue
      |  1
    3 | 10 continue
      |  2
Error: Duplicate statement label 10 at (1) and (2)

where the '1' and '2' annotating lines 2 and 3 in the source are
colored red and green respectively, but the "(1)" and "(2)" in the
"Error" line are not colored - only the word "Error" in that line is
colored (red).

PR fortran/91426 covers this inconsistency, and there was some
debate there on ways we could address it.

Within the diagnostics subsystem as of GCC 6 onwards, a
rich_location can contain multiple location_t values.  The first is
considered the "primary" location, subsequent ones are considered
"secondary" locations.

diagnostic_show_locus colorizes locations in the annotated source
lines by using the "diagnostic_kind" color for the primary
location_t within the rich_location, and then alternating between
two other colors for any secondary location_t values within the
rich_location ("range1" and "range2" within GCC_COLORS).

fortran/error.c currently supports at most 2 locations per
diagnostic (via the %L formatting code).

Hence fortran diagnostics have a primary location_t, and some have
a single secondary location_t.

Based on discussion with tkoenig in the bug report, this patch
tweaks fortran's error.c so that it colorizes the "(1)" and "(2)" in
the "Error" line, with their colors matching those of the "1" and
"2" in the source line annotations.

For a single-%L diagnostic, it colorizes the single "(1)" so that
its color matches that of the "1" in the source line annotation.

My view is that this makes the overall output more readable, making
the association between the text of the diagnostic and the
annotated source clearer.

For example, if you have multiple diagnostics with a mixture of
warnings and errors, the differences in colorization make it
clearer (to me, at least) what's being referred to in the text
of each diagnostic (e.g. I've been testing with
gfortran.dg/achar_3.f90 -Wall).

As normal, no colorization is done if colorization is disabled
e.g. via '-fdiagnostics-color=never' or if stderr isn't going to a
tty.

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

I think technically I can self-approve this, but I'm not a
day-to-day user of fortran; does this look sane?

Thanks
David

gcc/fortran/ChangeLog:
	PR fortran/91426
	* error.c (curr_diagnostic): New static variable.
	(gfc_report_diagnostic): New static function.
	(gfc_warning): Replace call to diagnostic_report_diagnostic with
	call to gfc_report_diagnostic.
	(gfc_format_decoder): Colorize the text of %L and %C to match the
	colorization used by diagnostic_show_locus.
	(gfc_warning_now_at): Replace call to diagnostic_report_diagnostic with
	call to gfc_report_diagnostic.
	(gfc_warning_now): Likewise.
	(gfc_warning_internal): Likewise.
	(gfc_error_now): Likewise.
	(gfc_fatal_error): Likewise.
	(gfc_error_opt): Likewise.
	(gfc_internal_error): Likewise.
---
 gcc/fortran/error.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

Comments

Thomas Koenig Sept. 21, 2019, 1:24 p.m. UTC | #1
Hi David,

> I think technically I can self-approve this, but I'm not a
> day-to-day user of fortran; does this look sane?

Very much so, I also find this more readable.

I'd wait another day or so for comitting this, so that other people
with different aesthetic sense can also chime in if they want to :-)

Regards

	Thomas
Thomas Koenig Sept. 25, 2019, 4:32 p.m. UTC | #2
Hi David,

> does this look sane?

Yes.

OK for trunk, and thanks a lot!

Regards

	Thomas

Patch
diff mbox series

diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c
index 68a2791..a0ce7a6 100644
--- a/gcc/fortran/error.c
+++ b/gcc/fortran/error.c
@@ -760,6 +760,23 @@  gfc_clear_pp_buffer (output_buffer *this_buffer)
   global_dc->last_location = UNKNOWN_LOCATION;
 }
 
+/* The currently-printing diagnostic, for use by gfc_format_decoder,
+   for colorizing %C and %L.  */
+
+static diagnostic_info *curr_diagnostic;
+
+/* A helper function to call diagnostic_report_diagnostic, while setting
+   curr_diagnostic for the duration of the call.  */
+
+static bool
+gfc_report_diagnostic (diagnostic_info *diagnostic)
+{
+  gcc_assert (diagnostic != NULL);
+  curr_diagnostic = diagnostic;
+  bool ret = diagnostic_report_diagnostic (global_dc, diagnostic);
+  curr_diagnostic = NULL;
+  return ret;
+}
 
 /* This is just a helper function to avoid duplicating the logic of
    gfc_warning.  */
@@ -789,7 +806,7 @@  gfc_warning (int opt, const char *gmsgid, va_list ap)
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc,
 		       DK_WARNING);
   diagnostic.option_index = opt;
-  bool ret = diagnostic_report_diagnostic (global_dc, &diagnostic);
+  bool ret = gfc_report_diagnostic (&diagnostic);
 
   if (buffered_p)
     {
@@ -954,7 +971,18 @@  gfc_format_decoder (pretty_printer *pp, text_info *text, const char *spec,
 						 loc->lb->location,
 						 offset);
 	text->set_location (loc_num, src_loc, SHOW_RANGE_WITH_CARET);
+	/* Colorize the markers to match the color choices of
+	   diagnostic_show_locus (the initial location has a color given
+	   by the "kind" of the diagnostic, the secondary location has
+	   color "range1").  */
+	gcc_assert (curr_diagnostic != NULL);
+	const char *color
+	  = (loc_num
+	     ? "range1"
+	     : diagnostic_get_color_for_kind (curr_diagnostic->kind));
+	pp_string (pp, colorize_start (pp_show_color (pp), color));
 	pp_string (pp, result[loc_num]);
+	pp_string (pp, colorize_stop (pp_show_color (pp)));
 	return true;
       }
     default:
@@ -1153,7 +1181,7 @@  gfc_warning_now_at (location_t loc, int opt, const char *gmsgid, ...)
   va_start (argp, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_WARNING);
   diagnostic.option_index = opt;
-  ret = diagnostic_report_diagnostic (global_dc, &diagnostic);
+  ret = gfc_report_diagnostic (&diagnostic);
   va_end (argp);
   return ret;
 }
@@ -1172,7 +1200,7 @@  gfc_warning_now (int opt, const char *gmsgid, ...)
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc,
 		       DK_WARNING);
   diagnostic.option_index = opt;
-  ret = diagnostic_report_diagnostic (global_dc, &diagnostic);
+  ret = gfc_report_diagnostic (&diagnostic);
   va_end (argp);
   return ret;
 }
@@ -1191,7 +1219,7 @@  gfc_warning_internal (int opt, const char *gmsgid, ...)
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc,
 		       DK_WARNING);
   diagnostic.option_index = opt;
-  ret = diagnostic_report_diagnostic (global_dc, &diagnostic);
+  ret = gfc_report_diagnostic (&diagnostic);
   va_end (argp);
   return ret;
 }
@@ -1209,7 +1237,7 @@  gfc_error_now (const char *gmsgid, ...)
 
   va_start (argp, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_ERROR);
-  diagnostic_report_diagnostic (global_dc, &diagnostic);
+  gfc_report_diagnostic (&diagnostic);
   va_end (argp);
 }
 
@@ -1225,7 +1253,7 @@  gfc_fatal_error (const char *gmsgid, ...)
 
   va_start (argp, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_FATAL);
-  diagnostic_report_diagnostic (global_dc, &diagnostic);
+  gfc_report_diagnostic (&diagnostic);
   va_end (argp);
 
   gcc_unreachable ();
@@ -1310,7 +1338,7 @@  gfc_error_opt (int opt, const char *gmsgid, va_list ap)
     }
 
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &richloc, DK_ERROR);
-  diagnostic_report_diagnostic (global_dc, &diagnostic);
+  gfc_report_diagnostic (&diagnostic);
 
   if (buffered_p)
     {
@@ -1360,7 +1388,7 @@  gfc_internal_error (const char *gmsgid, ...)
 
   va_start (argp, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_ICE);
-  diagnostic_report_diagnostic (global_dc, &diagnostic);
+  gfc_report_diagnostic (&diagnostic);
   va_end (argp);
 
   gcc_unreachable ();