[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 Richard Biener changed: What|Removed |Added Status|NEW |ASSIGNED
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 David Malcolm changed: What|Removed |Added Target Milestone|10.3|--- Status|WAITING |NEW --- Comment #24 from David Malcolm --- Clearing the milestone field. I think the only remaining things are (still): > (In reply to David Malcolm from comment #0) > [...] > > * the diagnostic and the followup notes are grouped, so it's easier to pick > > out what messages relate to what > [...] > > IIRC, clang does something where the left-hand column is only non-empty for > > the start of a diagnostic; followup lines (e.g. due to line wrapping) are > > indented by 1 char, so that the "wall of text" is broken up somewhat > [...]
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #23 from Jonathan Wakely --- Dave, should we unset the milestone? Any further changes will only happen on trunk not the gcc-10 branch, right?
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 Richard Biener changed: What|Removed |Added Target Milestone|10.2|10.3 --- Comment #22 from Richard Biener --- GCC 10.2 is released, adjusting target milestone.
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 Eric Gallager changed: What|Removed |Added Status|ASSIGNED|WAITING CC||egallager at gcc dot gnu.org --- Comment #21 from Eric Gallager --- (In reply to Eric Gallager from comment #19) > (In reply to Martin Liška from comment #18) > > @David: Can we close this now? > > I'm guessing he's probably waiting for his static analyzer to be merged; > that patch series looked like it had some stuff relevant to this bug in it ok, the static analyzer has been merged... so, David, can we close this now?
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 Jakub Jelinek changed: What|Removed |Added Target Milestone|10.0|10.2 --- Comment #20 from Jakub Jelinek --- GCC 10.1 has been released.
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #19 from Eric Gallager --- (In reply to Martin Liška from comment #18) > @David: Can we close this now? I'm guessing he's probably waiting for his static analyzer to be merged; that patch series looked like it had some stuff relevant to this bug in it
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #18 from Martin Liška --- @David: Can we close this now?
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #17 from Eric Gallager --- (In reply to David Malcolm from comment #16) > (In reply to Martin Liška from comment #14) > > David: Can the bug be marked as resolved? > > Much of this is implemented for gcc 9. > > I want to keep this open, to revisit it in gcc 10. It's gcc 10 now. > I think the main pending items are: > > (In reply to David Malcolm from comment #0) > [...] > > * the diagnostic and the followup notes are grouped, so it's easier to pick > > out what messages relate to what > [...] > > IIRC, clang does something where the left-hand column is only non-empty for > > the start of a diagnostic; followup lines (e.g. due to line wrapping) are > > indented by 1 char, so that the "wall of text" is broken up somewhat > [...]
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 David Malcolm changed: What|Removed |Added Status|WAITING |ASSIGNED Target Milestone|9.0 |10.0 --- Comment #16 from David Malcolm --- (In reply to Martin Liška from comment #14) > David: Can the bug be marked as resolved? Much of this is implemented for gcc 9. I want to keep this open, to revisit it in gcc 10. I think the main pending items are: (In reply to David Malcolm from comment #0) [...] > * the diagnostic and the followup notes are grouped, so it's easier to pick > out what messages relate to what [...] > IIRC, clang does something where the left-hand column is only non-empty for > the start of a diagnostic; followup lines (e.g. due to line wrapping) are > indented by 1 char, so that the "wall of text" is broken up somewhat [...]
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 Eric Gallager changed: What|Removed |Added Status|ASSIGNED|WAITING --- Comment #15 from Eric Gallager --- (In reply to Martin Liška from comment #14) > David: Can the bug be marked as resolved? WAITING on a reply
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 Martin Liška changed: What|Removed |Added CC||marxin at gcc dot gnu.org --- Comment #14 from Martin Liška --- David: Can the bug be marked as resolved?
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #13 from David Malcolm --- See also the various changes I've made in response to PR 87091.
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #12 from David Malcolm --- Another commit related to this: r263564 (add labeling of source ranges)
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889
--- Comment #11 from David Malcolm ---
Author: dmalcolm
Date: Mon Aug 20 21:06:46 2018
New Revision: 263675
URL: https://gcc.gnu.org/viewcvs?rev=263675=gcc=rev
Log:
Add support for grouping of related diagnostics (PR other/84889)
We often emit logically-related groups of diagnostics.
A relatively simple case is this:
if (warning_at (body_loc, OPT_Wmultistatement_macros,
"macro expands to multiple statements"))
inform (guard_loc, "some parts of macro expansion are not guarded by "
"this %qs clause", guard_tinfo_to_string (keyword));
where the "note" diagnostic issued by the "inform" call
is guarded by the -Wmultistatement_macros warning.
More complicated examples can be seen in the C++ frontend,
where e.g. print_z_candidates can lead to numerous "note"
diagnostics being emitted.
I'm looking at various ways to improve how we handle such related
diagnostics, but, prior to this patch, there was no explicit
relationship between these diagnostics: the diagnostics subsystem
had no way of "knowing" that these were related.
This patch introduces a simple way to group the diagnostics:
an auto_diagnostic_group class: all diagnostics emitted within
the lifetime of an auto_diagnostic_group instance are logically
grouped.
Hence in the above example, the two diagnostics can be grouped
by simply adding an auto_diagnostic_group instance:
auto_diagnostic_group d;
if (warning_at (body_loc, OPT_Wmultistatement_macros,
"macro expands to multiple statements"))
inform (guard_loc, "some parts of macro expansion are not guarded by "
"this %qs clause", guard_tinfo_to_string (keyword));
Some more awkard cases are of the form:
if (some_condition
&& warning_at (...)
&& more_conditions)
inform (...);
which thus need restructuring to:
if (some_condition)
{
auto_diagnostic_group d;
warning_at (...);
if (more_conditions)
inform (...);
}
so that the lifetime of the auto_diagnostic_group groups the
warning_at and the inform call.
Nesting is handled by simply tracking a nesting depth within the
diagnostic_context.: all diagnostics are treated as grouped until the
final auto_diagnostic_group is popped.
diagnostic.c uses this internally, so that all diagnostics are part of
a group - those that are "by themselves" are treated as being part of
a group with one element.
The diagnostic_context gains optional callbacks for displaying the
start of a group (when the first diagnostic is emitted within it), and
the end of a group (for when the group was non-empty); these callbacks
are unused by default, but a test plugin demonstrates them (and verifies
that the machinery is working).
As noted above, I'm looking at various ways to use the grouping to
improve how we output the diagnostics.
FWIW, I experimented with a more involved implementation, of the form:
diagnostic_group d;
if (d.warning_at (body_loc, OPT_Wmultistatement_macros,
"macro expands to multiple statements"))
d.inform (guard_loc, "some parts of macro expansion are not guarded by "
"this %qs clause", guard_tinfo_to_string (keyword));
which had the advantage of allowing auto-detection of the places where
groups were needed (by converting ::warning_at's return type to bool),
but it was a much more invasive patch, especially when dealing with
the places in the C++ frontend that can emit numerous notes after
an error or warning (and thus having to pass the group around)
Hence I went with this simpler approach.
gcc/c-family/ChangeLog:
PR other/84889
* c-attribs.c (common_handle_aligned_attribute): Add
auto_diagnostic_group instance.
* c-indentation.c (warn_for_misleading_indentation): Likewise.
* c-opts.c (c_common_post_options): Likewise.
* c-warn.c (warn_logical_not_parentheses): Likewise.
(warn_duplicated_cond_add_or_warn): Likewise.
(warn_for_multistatement_macros): Likewise.
gcc/c/ChangeLog:
PR other/84889
* c-decl.c (pushtag): Add auto_diagnostic_group instance.
(diagnose_mismatched_decls): Likewise, in various places.
(warn_if_shadowing): Likewise.
(implicit_decl_warning): Likewise.
(implicitly_declare): Likewise.
(undeclared_variable): Likewise.
(declare_label): Likewise.
(grokdeclarator): Likewise.
(start_function): Likewise.
* c-parser.c (c_parser_declaration_or_fndef): Likewise.
(c_parser_parameter_declaration): Likewise.
(c_parser_binary_expression): Likewise.
* c-typeck.c (c_expr_sizeof_expr): Likewise.
(parser_build_binary_op): Likewise.
(build_unary_op): Likewise.
(error_init): Likewise.
(pedwarn_init): Likewise.
(warning_init): Likewise.
(convert_for_assignment): Likewise.
gcc/cp/ChangeLog:
PR other/84889
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #10 from David Malcolm --- Author: dmalcolm Date: Thu Aug 9 15:32:13 2018 New Revision: 263450 URL: https://gcc.gnu.org/viewcvs?rev=263450=gcc=rev Log: diagnostics: add line numbers to source (PR other/84889) This patch adds a left margin to the lines of source (and annotations) printed by diagnostic_show_locus, so that e.g. rather than: test.c: In function 'test': test.c:12:15: error: 'struct foo' has no member named 'm_bar'; did you mean 'bar'? return ptr->m_bar; ^ bar we print: test.c: In function 'test': test.c:12:15: error: 'struct foo' has no member named 'm_bar'; did you mean 'bar'? 12 | return ptr->m_bar; | ^ | bar Similarly, for a multiline case (in C++ this time), this: bad-binary-ops.C: In function 'int test_2()': bad-binary-ops.C:26:4: error: no match for 'operator+' (operand types are 's' and 't') return (some_function () + some_other_function ()); ^~~~ becomes: bad-binary-ops.C: In function 'int test_2()': bad-binary-ops.C:26:4: error: no match for 'operator+' (operand types are 's' and 't') 25 | return (some_function () | 26 |+ some_other_function ()); |^~~~ I believe this slightly improves the readability of the output, in that it: - distinguishes between the user's source code vs the annotation lines that we're adding (the underlinings and fix-it hints here) - shows the line numbers in another place (potentially helpful for multiline diagnostics, where the user can see the line numbers directly, rather than have to figure them out relative to the caret: in the 2nd example, note how the diagnostic is reported at line 26, but the first line printed is actually line 25) I'm not sure that this is the precise format we want to go with [1], but I think it's an improvement over the status quo, and we're in stage 1 of gcc 9, so there's plenty of time to shake out issues. I've turned it on by default; it can be disabled via -fno-diagnostics-show-line-numbers (it's also turned off in the testsuite, to avoid breaking numerous existing test cases). [1] Some possible variants: - maybe just "LL|" rather than "LL | " - maybe ':' rather than '|' - maybe we should have some leading indentation, to better split up the diagnostics visually via the left-hand column - etc gcc/ChangeLog: PR other/84889 * common.opt (fdiagnostics-show-line-numbers): New option. * diagnostic-show-locus.c (class layout): Add fields "m_show_line_numbers_p" and "m_linenum_width"; (num_digits): New function. (test_num_digits): New function. (layout::layout): Initialize new fields. Update m_x_offset logic to handle any left margin. (layout::print_source_line): Print line number when requested. (layout::start_annotation_line): New member function. (layout::print_annotation_line): Call it. (layout::print_leading_fixits): Likewise. (layout::print_trailing_fixits): Likewise. Update calls to move_to_column for new parameter. (layout::get_x_bound_for_row): Add "add_left_margin" param and use it to potentially call start_annotation_line. (layout::show_ruler): Call start_annotation_line. (selftest::test_line_numbers_multiline_range): New selftest. (selftest::diagnostic_show_locus_c_tests): Call test_num_digits and selftest::test_line_numbers_multiline_range. * diagnostic.c (diagnostic_initialize): Initialize show_line_numbers_p. * diagnostic.h (struct diagnostic_context): Add field "show_line_numbers_p". * doc/invoke.texi (Diagnostic Message Formatting Options): Add -fno-diagnostics-show-line-numbers. * dwarf2out.c (gen_producer_string): Add OPT_fdiagnostics_show_line_numbers to the ignored options. * lto-wrapper.c (merge_and_complain): Likewise to the "pick one setting" options. (append_compiler_options): Likewise to the dropped options. (append_diag_options): Likewise to the passed-on options. * opts.c (common_handle_option): Handle the new option. * toplev.c (general_init): Set up global_dc->show_line_numbers_p. gcc/testsuite/ChangeLog: PR other/84889 * gcc.dg/plugin/diagnostic-test-show-locus-bw-line-numbers.c: New test. * gcc.dg/plugin/diagnostic-test-show-locus-color-line-numbers.c: New test. * gcc.dg/plugin/plugin.exp (plugin_test_list): Add the new tests. * lib/prune.exp: Add -fno-diagnostics-show-line-numbers to TEST_ALWAYS_FLAGS. Added: trunk/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw-line-numbers.c trunk/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color-line-numbers.c
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 Eric Gallager changed: What|Removed |Added Status|NEW |ASSIGNED --- Comment #9 from Eric Gallager --- ASSIGNED since there's an assignee
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #8 from David Malcolm --- But what about parallel builds, where the errors can get interleaved?
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #7 from David Malcolm --- (In reply to David Malcolm from comment #5) > Note to self: need to look at Elm. http://elm-lang.org/blog/compiler-errors-for-humans http://elm-lang.org/blog/compilers-as-assistants
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 Eric Gallager changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2018-03-16 Ever confirmed|0 |1 --- Comment #6 from Eric Gallager --- (In reply to David Malcolm from comment #4) > (In reply to Eric Gallager from comment #3) > > Link for this next one though? > https://news.ycombinator.com/item?id=16598074 (In reply to David Malcolm from comment #5) > ...and according to: > https://www.reddit.com/r/cpp/comments/84op5c/usability_improvements_in_gcc_8/ > dvspoet/ > the rust developers took ideas on diagnostics-printing from Elm. > > Note to self: need to look at Elm. Thanks, confirmed.
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #5 from David Malcolm --- ...and according to: https://www.reddit.com/r/cpp/comments/84op5c/usability_improvements_in_gcc_8/dvspoet/ the rust developers took ideas on diagnostics-printing from Elm. Note to self: need to look at Elm.
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #4 from David Malcolm --- (In reply to Eric Gallager from comment #3) > Link for this next one though? https://news.ycombinator.com/item?id=16598074
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 Eric Gallager changed: What|Removed |Added CC||egallager at gcc dot gnu.org --- Comment #3 from Eric Gallager --- Thanks for these links: (In reply to David Malcolm from comment #0) > Quoting an example from: > > https://stackoverflow.com/questions/44003622/implementing-trait-for- > fnsomething-in-rust (In reply to David Malcolm from comment #1) > Example of Rust error (showing colorization): > https://i.redd.it/qm4oceuuckyy.png Link for this next one though? (In reply to David Malcolm from comment #2) > Hacker News user "earenndil" suggested: > > > What about a line of ─ in between different unassociated errors? > > So if there is a group of related errors that are about the same actual > > problem, or a note associated with an error, they're together, but then > > there's a line of coloured ─ separating them from the next set.
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #2 from David Malcolm --- Hacker News user "earenndil" suggested: > What about a line of ─ in between different unassociated errors? > So if there is a group of related errors that are about the same actual > problem, or a note associated with an error, they're together, but then > there's a line of coloured ─ separating them from the next set. It feels worth prototyping, at least, for gcc 9.
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 --- Comment #1 from David Malcolm --- Example of Rust error (showing colorization): https://i.redd.it/qm4oceuuckyy.png
[Bug other/84889] Ideas on revamping how we format diagnostics
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84889 David Malcolm changed: What|Removed |Added Target Milestone|--- |9.0
