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®rtested 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(-) 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 (); -- 1.8.5.3