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