On 12/6/24 5:52 PM, David Malcolm wrote:
On Fri, 2024-12-06 at 12:33 -0500, Jason Merrill wrote:
On 11/12/24 9:02 AM, David Malcolm wrote:
[from 0/3]
The most natural way to present textual output is to use
indentation,
so patch 1 does this. However, our existing textual format uses
the
source location as a prefix, e.g.
PATH/foo.cc: error: message goes here
and unfortunately this makes the indented hierarchy unreadable
(especially if a tree of diagnostics spans multiple source files).
I've previously tried to do a bit of diagnostic nesting with initial
spaces, which this patch removes. This seems like a small regression
in
the default mode, though I'm not too concerned about it.
Good catch.
Maybe there could be a mode that puts the indenting from this
framework
at the beginning of the message rather than the beginning of the
line?
Are you thinking of something like:
FILENAME:LINE:COLUMN: SEVERITY: <INDENT> MESSAGE [METADATA}
e.g.
foo.c:17:11: error: <INDENT>your code is suboptimal [-Wsomething]
where <INDENT> could be based on the nesting level?
I tried this, but it didn't work well. The issue with this is that the
indentation breaks down visually if the messages are coming from
different source files, or, indeed, if the number of digits used for
the line numbers and column numbers varies for different messages (all
of which tend to happen).
This patch uses the nested diagnostics capabilities added in the
earlier
patch in the C++ frontend.
With this, and enabling the non-standard text formatting via:
-fdiagnostics-set-output=text:experimental-nesting=yes
and using:
-std=c++20 -fconcepts-diagnostics-depth=2
then the output for the example in SG15's P3358R0 ("SARIF for
Structured
Diagnostics") is:
P3358R0.C: In function ‘int main()’:
P3358R0.C:26:6: error: no matching function for call to
‘pet(lizard)’
26 | pet(lizard{});
| ~~~^~~~~~~~~~
• note: candidate: ‘template<class auto:1> requires
pettable<auto:1> void pet(auto:1)’
P3358R0.C:21:6:
Putting the source location on a separate line seems like an
interesting
approach to the issue of variable-length pathnames vs. nesting.
In a quick test with nested-diagnostics-1-truncated.C, it seems like
emacs compile-mode has trouble with the indented source locations.
The
singly indented ones it doesn't recognize at all; the doubly indented
ones it correctly identifies as locations, but thinks the file name
is
e.g. "default nested-diagnostics-1-truncated.C:25". And it
misidentifies the line
• nested-diagnostics-1-truncated.C: In substitution of
‘template<class auto:1> requires pettable<auto:1> void \
pet(auto:1) [with auto:1 = lizard]’:
as the last :1 being a line number in a very long and strange
filename.
The lines with bullets don't work, but the lines below them work for me
in Emacs. I'm attaching a screenshot in the hope of clarifying.
In the attached, I moved the cursor in the compilation buffer to the
line:
../../src/gcc/testsuite/g++.dg/concepts/nested-diagnostics-1-truncated.C:25:6:
within
• required from here
../../src/gcc/testsuite/g++.dg/concepts/nested-diagnostics-1-truncated.C:25:6:
25 | pet(lizard{}); // { dg-error "no matching function for call to
'pet\\\(lizard\\\)'" }
| ~~~^~~~~~~~~~
and on pressing "return" in the compilation buffer, emacs took me to
the pertinent source code, line 25, column 6 (which emacs refers to as
column 5).
Interesting, perhaps it was easier for emacs to identify the file name
in your case because of the explicit directories, whereas my compilation
was just looking in the current directory.
Note that DejaGnu testcases inject:
experimental-nesting-show-locations=no
as part of the
-fdiagnostics-set-output=text:
which suppresses the printing of those extra lines.
This is a developer-facing option to make it easy to write DejaGnu
tests with dg-begin/end-multiline-output, since otherwise the output
would embed paths in various places, and DejaGnu passes absolute paths,
which would be a pain to deal with in dg directives for.
Hopefully the attached screenshot clarifies what I'm hoping end-users
to see (if they use
-fdiagnostics-set-output=text:experimental-nesting=yes
)
I wonder about an option to put source locations at the start of the
line while nesting the actual diagnostics? Possibly before the
quoted
source line, to conserve the actual number of lines of output?
21 | void pet(pettable auto t);
| ^~~
• note: template argument deduction/substitution failed:
• note: constraints not satisfied
• P3358R0.C: In substitution of ‘template<class auto:1>
requires pettable<auto:1> void pet(auto:1) [with auto:1 =
lizard]’:
• required from here
P3358R0.C:26:6:
26 | pet(lizard{});
| ~~~^~~~~~~~~~
• required for the satisfaction of ‘pettable<auto:1>’
[with auto:1 = lizard]
Is the behavior you're suggesting the behavior we now get without
experimental-nesting-show-locations=no
?
I was suggesting something like either
P3358R0.C:26:6:
26 | pet(lizard{});
| ~~~^~~~~~~~~~
or
P3358R0.C:26:6: pet(lizard{});
~~~^~~~~~~~~~
by "at the start of the line" I meant column 0.
Incidentally, the wording of these two context lines seems a bit
weird;
we're checking the satisfaction of pet<lizard>, not pettable<lizard>.
The latter is a constraint on the former.
P3358R0.C:19:9:
19 | concept pettable = has_member_pet<T> or
has_default_pet<T>;
| ^~~~~~~~
• note: no operand of the disjunction is satisfied
P3358R0.C:19:38:
19 | concept pettable = has_member_pet<T> or
has_default_pet<T>;
|
~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
• note: the operand ‘has_member_pet<T>’ is unsatisfied
because
P3358R0.C:19:20:
19 | concept pettable = has_member_pet<T> or
has_default_pet<T>;
|
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
• required for the satisfaction of ‘has_member_pet<T>’
[with T = lizard]
P3358R0.C:13:9:
13 | concept has_member_pet = requires(T t) {
t.pet(); };
| ^~~~~~~~~~~~~~
It would be nice to avoid two lines about has_member_pet in this
case.
• required for the satisfaction of ‘pettable<auto:1>’
[with auto:1 = lizard]
P3358R0.C:19:9:
19 | concept pettable = has_member_pet<T> or
has_default_pet<T>;
| ^~~~~~~~
It would be nice to do away with this redundant context that we
already
printed above.
• in requirements with ‘T t’ [with T = lizard]
P3358R0.C:13:26:
13 | concept has_member_pet = requires(T t) {
t.pet(); };
|
^~~~~~~~~~~~~~~~~~~~~~~~~~
• note: the required expression ‘t.pet()’ is invalid,
because
P3358R0.C:13:47:
13 | concept has_member_pet = requires(T t) {
t.pet(); };
|
~~~~~^~
• error: ‘struct lizard’ has no member named ‘pet’
P3358R0.C:13:44:
13 | concept has_member_pet = requires(T t) {
t.pet(); };
|
~~^~~
• note: the operand ‘has_default_pet<T>’ is unsatisfied
because
P3358R0.C:19:41:
19 | concept pettable = has_member_pet<T> or
has_default_pet<T>;
|
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
• required for the satisfaction of
‘has_default_pet<T>’ [with T = lizard]
P3358R0.C:16:9:
16 | concept has_default_pet = T::is_pettable;
| ^~~~~~~~~~~~~~~
• required for the satisfaction of ‘pettable<auto:1>’
[with auto:1 = lizard]
P3358R0.C:19:9:
19 | concept pettable = has_member_pet<T> or
has_default_pet<T>;
| ^~~~~~~~
There's also the broader issue of weird context ordering in g++,
where
sometimes we get context from outside in (as expressed by the
nesting)
and sometimes from inside out (the "required for" lines), mixed
together. Maybe we could try to avoid printing inside-out context
(i.e.
everything added by cp_diagnostic_text_starter) at all when we're
already printing outside-in context? Or use formatting to
distinguish
it more clearly?
FWIW I find the current wording really unclear, but it's been hiding as
part of a sea of text for so long that I think I've been mentally
shutting down rather than engaging with the messages. I find the
indentation makes it *much* easier for me to grok what's being emitted.
I experimented a bit with rewriting this (e.g. for PR c++/106256), but
I confess I got lost in the various parts of global state in
cp/error.cc and pt.cc, such as maybe_print_instantiation_context which
if I'm reading things right uses current_tinst_level in pt.cc.
Maybe something for me to try to tackle in GCC 16?
Sure. Though I was thinking it might be simple enough to (optionally)
suppress the added context from cp_diagnostic_text_starter for a nested
diagnostic?
I'm hoping you could take a look at these followups:
[PATCH 1/2] c++: print z candidate count and number them
https://gcc.gnu.org/pipermail/gcc-patches/2024-November/669035.html
Replied separately.
[PATCH 2/2] diagnostics: suppress "note: " prefix in nested diagnostics
[PR116253]
https://gcc.gnu.org/pipermail/gcc-patches/2024-November/669034.html
(though I can arguably self-approve the latter)
This seems like it could have been part of patch 1/3 of this series, so
I agree it makes sense for you to self-approve it.
Jason
