[v2,RFC] c++: add color to function decl printing
Commit Message
On 12/13/21 06:02, Jonathan Wakely wrote:
> On Sun, 12 Dec 2021 at 05:39, Jason Merrill <jason@redhat.com
> <mailto:jason@redhat.com>> wrote:
> >
> > In reading C++ diagnostics, it's often hard to find the name of the
> function
> > in the middle of the template header, return type, parameters, and
> template
> > arguments. So let's colorize it, and maybe the template argument
> bindings
> > while we're at it.
> >
> > I've somewhat arbitrarily chosen bold green for the function name, and
> > non-bold magenta for the template arguments. I'm not at all attached to
> > these choices.
> >
> > A side-effect is that when this happens in a quote (i.e. %qD), the
> > rest of the quote after the function name is no longer bold. I think
> that's
> > acceptable; returning to the bold would require maintaining a
> colorize stack
> > instead of the on/off controls we have now.
> >
> > Any thoughts?
>
> I thought I was going to love this ... and it's a nice little
> improvement, but I didn't love it as much as I expected.
>
> Is it intentional that only the last function in the instantiation stack
> gets colourized? In this example the function on line 390 isn't highlighted:
>
> /home/jwakely/gcc/12/include/c++/12.0.0/bits/ranges_base.h:390:12: required
> by substitution of 'template<class _Tp> requires
> (is_bounded_array_v<typename std::remove_reference<_Tp>::type>) ||
> (__member_size*<_Tp>) || (__adl_size<_Tp>) || (__sentinel_size<_Tp>)
> constexpr auto std::ranges::__cust_access::_Size::operator()(_Tp&&)
> const [with _Tp = adl::Footie (&)[]]*'
Oops, I needed to change subst_to_string as well. Updated patch below.
> Aside: it's a little odd that the first caret diagnostic there only
> highlights the word "operator" and not the name of the function,
> "operator()".
Yes, I imagine we need to adjust DECL_SOURCE_LOCATION to use a range
instead of assuming the location of the first token is sufficient.
Jason
Comments
On Mon, 2021-12-13 at 09:58 -0500, Jason Merrill via Gcc-patches wrote:
> On 12/13/21 06:02, Jonathan Wakely wrote:
> > On Sun, 12 Dec 2021 at 05:39, Jason Merrill <jason@redhat.com
> > <mailto:jason@redhat.com>> wrote:
> > >
> > > In reading C++ diagnostics, it's often hard to find the name of
> > the
> > function
> > > in the middle of the template header, return type, parameters, and
> > template
> > > arguments. So let's colorize it, and maybe the template argument
> > bindings
> > > while we're at it.
Thanks for the patch; sorry for not responding before.
Overall I like that patch, with some reservations...
> > >
> > > I've somewhat arbitrarily chosen bold green for the function name,
> > and
> > > non-bold magenta for the template arguments. I'm not at all
> > attached to
> > > these choices.
I tried the patch with gnome terminals "light" and "dark" themes, and
the colors seemed readable with both color schemes.
> > >
> > > A side-effect is that when this happens in a quote (i.e. %qD), the
> > > rest of the quote after the function name is no longer bold. I
> > think
> > that's
> > > acceptable; returning to the bold would require maintaining a
> > colorize stack
> > > instead of the on/off controls we have now.
I was going to grumble about this, but having tried it on some
examples, I think it's actually an improvement in readability to drop
the emboldening, in that it reduces the "wall of bold text" seen.
Having tried it on some examples, I think the patch as a whole is a
definite readability win, in that it breaks up long stretches of bold
text into useful colorized parts; the result seems much easier to
decipher at a glance. I wonder to what extent this is a poor-man's
syntax highlighting?
Did you try any other variants of the highlighting? This approach
seems to work well, FWIW, I wonder if others might work better, or if
this is a local maxima in terms of readability.
I'm taking the liberty of attaching a screenshot (137K) showing
before/after the patch for the sake of discussion.
> > >
> > > Any thoughts?
I was also concerned about how this would interact with the template
type diff printing from f012c8ef4b35dcee9b5a3807868d050812d5b3b9, but I
did a few tests and it seems to work OK.
I only tested in lightly for a few minutes, so it could do with some
more testing.
> >
> > I thought I was going to love this ... and it's a nice little
> > improvement, but I didn't love it as much as I expected.
> >
> > Is it intentional that only the last function in the instantiation
> > stack
> > gets colourized? In this example the function on line 390 isn't
> > highlighted:
> >
> > /home/jwakely/gcc/12/include/c++/12.0.0/bits/ranges_base.h:390:12:
> > required
> > by substitution of 'template<class _Tp> requires
> > (is_bounded_array_v<typename std::remove_reference<_Tp>::type>) ||
> > (__member_size*<_Tp>) || (__adl_size<_Tp>) ||
> > (__sentinel_size<_Tp>)
> > constexpr auto std::ranges::__cust_access::_Size::operator()(_Tp&&)
> > const [with _Tp = adl::Footie (&)[]]*'
>
> Oops, I needed to change subst_to_string as well. Updated patch
> below.
Jonathan, did you try the v2 patch?
>
> > Aside: it's a little odd that the first caret diagnostic there only
> > highlights the word "operator" and not the name of the function,
> > "operator()".
>
> Yes, I imagine we need to adjust DECL_SOURCE_LOCATION to use a range
> instead of assuming the location of the first token is sufficient.
>
> Jason
[...]
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 9b4371b9213..cdfddd75343 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -4803,6 +4803,14 @@ SGR substring for location information,
@samp{file:line} or
> @vindex quote GCC_COLORS @r{capability}
> SGR substring for information printed within quotes.
>
> +@item fnname=
> +@vindex fnname GCC_COLORS @r{capability}
> +SGR substring for names of C++ functions.
> +
> +@item targs=
> +@vindex targs GCC_COLORS @r{capability}
> +SGR substring for C++ function template parameter bindings.
> +
> @item fixit-insert=
> @vindex fixit-insert GCC_COLORS @r{capability}
> SGR substring for fix-it hints suggesting text to
There's a section of the docs for -fdiagnostics-color a little above
this, starting
"The default @env{GCC_COLORS} is"
which needs to be updated whenever we add new color capabilities.
> diff --git a/gcc/cp/error.c b/gcc/cp/error.c
> index 323643d1e6f..0008ee2ee8d 100644
> --- a/gcc/cp/error.c
> +++ b/gcc/cp/error.c
> @@ -1,4 +1,4 @@
> -/* Call-backs for C++ error reporting.
> +/* Call-backs for -*- C++ -*- error reporting.
This change isn't called out in the ChangeLog. Is it deliberate?
[...]
> @@ -1158,6 +1163,22 @@ dump_simple_decl (cxx_pretty_printer *pp, tree
t, tree type, int flags)
> dump_type_suffix (pp, type, flags);
> }
>
> +struct colorize_guard
> +{
> + bool colorize;
> + cxx_pretty_printer *pp;
> +
> + colorize_guard (bool _colorize, cxx_pretty_printer *pp, const char
*name)
> + : colorize (_colorize && pp_show_color (pp)), pp (pp)
> + {
> + pp_string (pp, colorize_start (colorize, name));
> + }
> + ~colorize_guard ()
> + {
> + pp_string (pp, colorize_stop (colorize));
> + }
> +};
Might as well make this a class, and make the fields be private since
nothing accesses them, I think.
> @@ -1928,6 +1949,11 @@ dump_exception_spec (cxx_pretty_printer *pp,
tree t, int flags)
> static void
> dump_function_name (cxx_pretty_printer *pp, tree t, int flags)
> {
> + bool colorize = flags & (TFF_DECL_SPECIFIERS | TFF_RETURN_TYPE
> + | TFF_TEMPLATE_HEADER);
I'm not as familiar with this code as you, so I'm assuming the above is
correct. Maybe a comment explaining the intent of this logic?
> +
> + colorize_guard g (colorize, pp, "fnname");
> +
> tree name = DECL_NAME (t);
>
> /* We can get here with a decl that was synthesized by language-
[...]
> @@ -3598,20 +3628,20 @@ function_category (tree fn)
> && DECL_FUNCTION_MEMBER_P (fn))
> {
> if (DECL_STATIC_FUNCTION_P (fn))
> - return _("In static member function %qs");
> + return _("In static member function %qD");
> else if (DECL_COPY_CONSTRUCTOR_P (fn))
> - return _("In copy constructor %qs");
> + return _("In copy constructor %qD");
> else if (DECL_CONSTRUCTOR_P (fn))
> - return _("In constructor %qs");
> + return _("In constructor %qD");
> else if (DECL_DESTRUCTOR_P (fn))
> - return _("In destructor %qs");
> + return _("In destructor %qD");
> else if (LAMBDA_FUNCTION_P (fn))
> return _("In lambda function");
> else
> - return _("In member function %qs");
> + return _("In member function %qD");
> }
> else
> - return _("In function %qs");
> + return _("In function %qD");
> }
The leading comment for function_category still refers to %qs and thus
needs updating.
> @@ -4393,7 +4423,7 @@ cp_printer (pretty_printer *pp, text_info
*text, const char *spec,
> break;
> }
> }
> - result = decl_to_string (temp, verbose);
> + result = decl_to_string (temp, verbose, pp_show_color (pp));
> }
> break;
> case 'E': result = expr_to_string (next_tree); break;
[...]
FWIW there's no automated test coverage for this; there are some
examples of testing colorization in DejaGnu in the plugins
subdirectories e.g.
gcc/testsuite/g++.dg/plugin/show_template_tree_color_plugin.c
gcc/testsuite/g++.dg/plugin/show-template-tree-color.C
They can be awkward to write and maintain, but this stuff is non-
trivial, so it would be nice to have a test to make sure we don't
regress.
To what extent have you been "eating your own dogfood" with this patch?
(i.e. having it applied to your primary compiler for development). I'm
a bit wary of making this change at this point in the release cycle
without spending some time "living with it" (and sorry again for not
commenting on it earlier; I was avoiding being online for most of
December)
My feeling is that this patch would be OK with the above nits fixed,
given that it's relatively trivial to revert if it causes difficulties,
but we should test it "in anger".
Hope this is constructive
Dave
On Fri, 14 Jan 2022 at 21:49, David Malcolm wrote:
>
> Jonathan, did you try the v2 patch?
>
No, sorry.
On 1/14/22 16:49, David Malcolm wrote:
> On Mon, 2021-12-13 at 09:58 -0500, Jason Merrill via Gcc-patches wrote:
>> On 12/13/21 06:02, Jonathan Wakely wrote:
>>> On Sun, 12 Dec 2021 at 05:39, Jason Merrill <jason@redhat.com
>>> <mailto:jason@redhat.com>> wrote:
>>> >
>>> > In reading C++ diagnostics, it's often hard to find the name of
>>> the
>>> function
>>> > in the middle of the template header, return type, parameters, and
>>> template
>>> > arguments. So let's colorize it, and maybe the template argument
>>> bindings
>>> > while we're at it.
>
> Thanks for the patch; sorry for not responding before.
>
> Overall I like that patch, with some reservations...
>
>>> >
>>> > I've somewhat arbitrarily chosen bold green for the function name,
>>> and
>>> > non-bold magenta for the template arguments. I'm not at all
>>> attached to
>>> > these choices.
>
> I tried the patch with gnome terminals "light" and "dark" themes, and
> the colors seemed readable with both color schemes.
>
>>> >
>>> > A side-effect is that when this happens in a quote (i.e. %qD), the
>>> > rest of the quote after the function name is no longer bold. I
>>> think
>>> that's
>>> > acceptable; returning to the bold would require maintaining a
>>> colorize stack
>>> > instead of the on/off controls we have now.
>
> I was going to grumble about this, but having tried it on some
> examples, I think it's actually an improvement in readability to drop
> the emboldening, in that it reduces the "wall of bold text" seen.
>
> Having tried it on some examples, I think the patch as a whole is a
> definite readability win, in that it breaks up long stretches of bold
> text into useful colorized parts; the result seems much easier to
> decipher at a glance. I wonder to what extent this is a poor-man's
> syntax highlighting?
I think it definitely is. Going farther in that direction would make
sense it future.
> Did you try any other variants of the highlighting? This approach
> seems to work well, FWIW, I wonder if others might work better, or if
> this is a local maxima in terms of readability.
Do you have any particular variants in mind? This was motivated by my
having trouble finding the name of the function in diagnostic output, so
that's the main thing I wanted to highlight.
> I'm taking the liberty of attaching a screenshot (137K) showing
> before/after the patch for the sake of discussion.
>
>>> >
>>> > Any thoughts?
>
> I was also concerned about how this would interact with the template
> type diff printing from f012c8ef4b35dcee9b5a3807868d050812d5b3b9, but I
> did a few tests and it seems to work OK.
>
> I only tested in lightly for a few minutes, so it could do with some
> more testing.
>
>>>
>>> I thought I was going to love this ... and it's a nice little
>>> improvement, but I didn't love it as much as I expected.
>>>
>>> Is it intentional that only the last function in the instantiation
>>> stack
>>> gets colourized? In this example the function on line 390 isn't
>>> highlighted:
>>>
>>> /home/jwakely/gcc/12/include/c++/12.0.0/bits/ranges_base.h:390:12:
>>> required
>>> by substitution of 'template<class _Tp> requires
>>> (is_bounded_array_v<typename std::remove_reference<_Tp>::type>) ||
>>> (__member_size*<_Tp>) || (__adl_size<_Tp>) ||
>>> (__sentinel_size<_Tp>)
>>> constexpr auto std::ranges::__cust_access::_Size::operator()(_Tp&&)
>>> const [with _Tp = adl::Footie (&)[]]*'
>>
>> Oops, I needed to change subst_to_string as well. Updated patch
>> below.
>
> Jonathan, did you try the v2 patch?
>
>>
>>> Aside: it's a little odd that the first caret diagnostic there only
>>> highlights the word "operator" and not the name of the function,
>>> "operator()".
>>
>> Yes, I imagine we need to adjust DECL_SOURCE_LOCATION to use a range
>> instead of assuming the location of the first token is sufficient.
>>
>> Jason
>
> [...]
>
>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index 9b4371b9213..cdfddd75343 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -4803,6 +4803,14 @@ SGR substring for location information,
> @samp{file:line} or
>> @vindex quote GCC_COLORS @r{capability}
>> SGR substring for information printed within quotes.
>>
>> +@item fnname=
>> +@vindex fnname GCC_COLORS @r{capability}
>> +SGR substring for names of C++ functions.
>> +
>> +@item targs=
>> +@vindex targs GCC_COLORS @r{capability}
>> +SGR substring for C++ function template parameter bindings.
>> +
>> @item fixit-insert=
>> @vindex fixit-insert GCC_COLORS @r{capability}
>> SGR substring for fix-it hints suggesting text to
>
> There's a section of the docs for -fdiagnostics-color a little above
> this, starting
> "The default @env{GCC_COLORS} is"
> which needs to be updated whenever we add new color capabilities.
Fixed.
>> diff --git a/gcc/cp/error.c b/gcc/cp/error.c
>> index 323643d1e6f..0008ee2ee8d 100644
>> --- a/gcc/cp/error.c
>> +++ b/gcc/cp/error.c
>> @@ -1,4 +1,4 @@
>> -/* Call-backs for C++ error reporting.
>> +/* Call-backs for -*- C++ -*- error reporting.
>
> This change isn't called out in the ChangeLog. Is it deliberate?
Yes, an incidental change to tell emacs that it's a C++ source file
despite the .c suffix. Probably not important, since it seems we're
about to rename the .c files.
> [...]
>
>> @@ -1158,6 +1163,22 @@ dump_simple_decl (cxx_pretty_printer *pp, tree
> t, tree type, int flags)
>> dump_type_suffix (pp, type, flags);
>> }
>>
>> +struct colorize_guard
>> +{
>> + bool colorize;
>> + cxx_pretty_printer *pp;
>> +
>> + colorize_guard (bool _colorize, cxx_pretty_printer *pp, const char
> *name)
>> + : colorize (_colorize && pp_show_color (pp)), pp (pp)
>> + {
>> + pp_string (pp, colorize_start (colorize, name));
>> + }
>> + ~colorize_guard ()
>> + {
>> + pp_string (pp, colorize_stop (colorize));
>> + }
>> +};
>
> Might as well make this a class, and make the fields be private since
> nothing accesses them, I think.
Done.
>> @@ -1928,6 +1949,11 @@ dump_exception_spec (cxx_pretty_printer *pp,
> tree t, int flags)
>> static void
>> dump_function_name (cxx_pretty_printer *pp, tree t, int flags)
>> {
>> + bool colorize = flags & (TFF_DECL_SPECIFIERS | TFF_RETURN_TYPE
>> + | TFF_TEMPLATE_HEADER);
>
> I'm not as familiar with this code as you, so I'm assuming the above is
> correct. Maybe a comment explaining the intent of this logic?
Done.
>> +
>> + colorize_guard g (colorize, pp, "fnname");
>> +
>> tree name = DECL_NAME (t);
>>
>> /* We can get here with a decl that was synthesized by language-
>
> [...]
>
>> @@ -3598,20 +3628,20 @@ function_category (tree fn)
>> && DECL_FUNCTION_MEMBER_P (fn))
>> {
>> if (DECL_STATIC_FUNCTION_P (fn))
>> - return _("In static member function %qs");
>> + return _("In static member function %qD");
>> else if (DECL_COPY_CONSTRUCTOR_P (fn))
>> - return _("In copy constructor %qs");
>> + return _("In copy constructor %qD");
>> else if (DECL_CONSTRUCTOR_P (fn))
>> - return _("In constructor %qs");
>> + return _("In constructor %qD");
>> else if (DECL_DESTRUCTOR_P (fn))
>> - return _("In destructor %qs");
>> + return _("In destructor %qD");
>> else if (LAMBDA_FUNCTION_P (fn))
>> return _("In lambda function");
>> else
>> - return _("In member function %qs");
>> + return _("In member function %qD");
>> }
>> else
>> - return _("In function %qs");
>> + return _("In function %qD");
>> }
>
> The leading comment for function_category still refers to %qs and thus
> needs updating.
Fixed.
>> @@ -4393,7 +4423,7 @@ cp_printer (pretty_printer *pp, text_info
> *text, const char *spec,
>> break;
>> }
>> }
>> - result = decl_to_string (temp, verbose);
>> + result = decl_to_string (temp, verbose, pp_show_color (pp));
>> }
>> break;
>> case 'E': result = expr_to_string (next_tree); break;
>
> [...]
>
> FWIW there's no automated test coverage for this; there are some
> examples of testing colorization in DejaGnu in the plugins
> subdirectories e.g.
> gcc/testsuite/g++.dg/plugin/show_template_tree_color_plugin.c
> gcc/testsuite/g++.dg/plugin/show-template-tree-color.C
> They can be awkward to write and maintain, but this stuff is non-
> trivial, so it would be nice to have a test to make sure we don't
> regress.
>
> To what extent have you been "eating your own dogfood" with this patch?
> (i.e. having it applied to your primary compiler for development). I'm
> a bit wary of making this change at this point in the release cycle
> without spending some time "living with it" (and sorry again for not
> commenting on it earlier; I was avoiding being online for most of
> December)
Not to a significant extent; this isn't so important for compiling
relatively simple C++ code like we have in GCC, and I mostly build with
the system compiler. I've mostly used it when debugging the compiler's
handling of a larger testcase.
> My feeling is that this patch would be OK with the above nits fixed,
> given that it's relatively trivial to revert if it causes difficulties,
> but we should test it "in anger".
>
> Hope this is constructive
Definitely, thanks. Here's a revision to address those nits:
On 1/14/22 22:56, Jason Merrill wrote:
> On 1/14/22 16:49, David Malcolm wrote:
>> On Mon, 2021-12-13 at 09:58 -0500, Jason Merrill via Gcc-patches wrote:
>>> On 12/13/21 06:02, Jonathan Wakely wrote:
>>>> On Sun, 12 Dec 2021 at 05:39, Jason Merrill <jason@redhat.com
>>>> <mailto:jason@redhat.com>> wrote:
>>>> >
>>>> > In reading C++ diagnostics, it's often hard to find the name of
>>>> the
>>>> function
>>>> > in the middle of the template header, return type, parameters, and
>>>> template
>>>> > arguments. So let's colorize it, and maybe the template argument
>>>> bindings
>>>> > while we're at it.
>>
>> Thanks for the patch; sorry for not responding before.
>>
>> Overall I like that patch, with some reservations...
>>
>>>> >
>>>> > I've somewhat arbitrarily chosen bold green for the function name,
>>>> and
>>>> > non-bold magenta for the template arguments. I'm not at all
>>>> attached to
>>>> > these choices.
>>
>> I tried the patch with gnome terminals "light" and "dark" themes, and
>> the colors seemed readable with both color schemes.
>>
>>>> >
>>>> > A side-effect is that when this happens in a quote (i.e. %qD), the
>>>> > rest of the quote after the function name is no longer bold. I
>>>> think
>>>> that's
>>>> > acceptable; returning to the bold would require maintaining a
>>>> colorize stack
>>>> > instead of the on/off controls we have now.
>>
>> I was going to grumble about this, but having tried it on some
>> examples, I think it's actually an improvement in readability to drop
>> the emboldening, in that it reduces the "wall of bold text" seen.
>>
>> Having tried it on some examples, I think the patch as a whole is a
>> definite readability win, in that it breaks up long stretches of bold
>> text into useful colorized parts; the result seems much easier to
>> decipher at a glance. I wonder to what extent this is a poor-man's
>> syntax highlighting?
>
> I think it definitely is. Going farther in that direction would make
> sense it future.
>
>> Did you try any other variants of the highlighting? This approach
>> seems to work well, FWIW, I wonder if others might work better, or if
>> this is a local maxima in terms of readability.
>
> Do you have any particular variants in mind? This was motivated by my
> having trouble finding the name of the function in diagnostic output, so
> that's the main thing I wanted to highlight.
>
>> I'm taking the liberty of attaching a screenshot (137K) showing
>> before/after the patch for the sake of discussion.
>>
>>>> >
>>>> > Any thoughts?
>>
>> I was also concerned about how this would interact with the template
>> type diff printing from f012c8ef4b35dcee9b5a3807868d050812d5b3b9, but I
>> did a few tests and it seems to work OK.
>>
>> I only tested in lightly for a few minutes, so it could do with some
>> more testing.
>>
>>>>
>>>> I thought I was going to love this ... and it's a nice little
>>>> improvement, but I didn't love it as much as I expected.
>>>>
>>>> Is it intentional that only the last function in the instantiation
>>>> stack
>>>> gets colourized? In this example the function on line 390 isn't
>>>> highlighted:
>>>>
>>>> /home/jwakely/gcc/12/include/c++/12.0.0/bits/ranges_base.h:390:12:
>>>> required
>>>> by substitution of 'template<class _Tp> requires
>>>> (is_bounded_array_v<typename std::remove_reference<_Tp>::type>) ||
>>>> (__member_size*<_Tp>) || (__adl_size<_Tp>) ||
>>>> (__sentinel_size<_Tp>)
>>>> constexpr auto std::ranges::__cust_access::_Size::operator()(_Tp&&)
>>>> const [with _Tp = adl::Footie (&)[]]*'
>>>
>>> Oops, I needed to change subst_to_string as well. Updated patch
>>> below.
>>
>> Jonathan, did you try the v2 patch?
>>
>>>
>>>> Aside: it's a little odd that the first caret diagnostic there only
>>>> highlights the word "operator" and not the name of the function,
>>>> "operator()".
>>>
>>> Yes, I imagine we need to adjust DECL_SOURCE_LOCATION to use a range
>>> instead of assuming the location of the first token is sufficient.
>>>
>>> Jason
>>
>> [...]
>>
>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>>> index 9b4371b9213..cdfddd75343 100644
>>> --- a/gcc/doc/invoke.texi
>>> +++ b/gcc/doc/invoke.texi
>>> @@ -4803,6 +4803,14 @@ SGR substring for location information,
>> @samp{file:line} or
>>> @vindex quote GCC_COLORS @r{capability}
>>> SGR substring for information printed within quotes.
>>> +@item fnname=
>>> +@vindex fnname GCC_COLORS @r{capability}
>>> +SGR substring for names of C++ functions.
>>> +
>>> +@item targs=
>>> +@vindex targs GCC_COLORS @r{capability}
>>> +SGR substring for C++ function template parameter bindings.
>>> +
>>> @item fixit-insert=
>>> @vindex fixit-insert GCC_COLORS @r{capability}
>>> SGR substring for fix-it hints suggesting text to
>>
>> There's a section of the docs for -fdiagnostics-color a little above
>> this, starting
>> "The default @env{GCC_COLORS} is"
>> which needs to be updated whenever we add new color capabilities.
>
> Fixed.
>
>>> diff --git a/gcc/cp/error.c b/gcc/cp/error.c
>>> index 323643d1e6f..0008ee2ee8d 100644
>>> --- a/gcc/cp/error.c
>>> +++ b/gcc/cp/error.c
>>> @@ -1,4 +1,4 @@
>>> -/* Call-backs for C++ error reporting.
>>> +/* Call-backs for -*- C++ -*- error reporting.
>>
>> This change isn't called out in the ChangeLog. Is it deliberate?
>
> Yes, an incidental change to tell emacs that it's a C++ source file
> despite the .c suffix. Probably not important, since it seems we're
> about to rename the .c files.
>
>> [...]
>>
>>> @@ -1158,6 +1163,22 @@ dump_simple_decl (cxx_pretty_printer *pp, tree
>> t, tree type, int flags)
>>> dump_type_suffix (pp, type, flags);
>>> }
>>> +struct colorize_guard
>>> +{
>>> + bool colorize;
>>> + cxx_pretty_printer *pp;
>>> +
>>> + colorize_guard (bool _colorize, cxx_pretty_printer *pp, const char
>> *name)
>>> + : colorize (_colorize && pp_show_color (pp)), pp (pp)
>>> + {
>>> + pp_string (pp, colorize_start (colorize, name));
>>> + }
>>> + ~colorize_guard ()
>>> + {
>>> + pp_string (pp, colorize_stop (colorize));
>>> + }
>>> +};
>>
>> Might as well make this a class, and make the fields be private since
>> nothing accesses them, I think.
>
> Done.
>
>>> @@ -1928,6 +1949,11 @@ dump_exception_spec (cxx_pretty_printer *pp,
>> tree t, int flags)
>>> static void
>>> dump_function_name (cxx_pretty_printer *pp, tree t, int flags)
>>> {
>>> + bool colorize = flags & (TFF_DECL_SPECIFIERS | TFF_RETURN_TYPE
>>> + | TFF_TEMPLATE_HEADER);
>>
>> I'm not as familiar with this code as you, so I'm assuming the above is
>> correct. Maybe a comment explaining the intent of this logic?
>
> Done.
>
>>> +
>>> + colorize_guard g (colorize, pp, "fnname");
>>> +
>>> tree name = DECL_NAME (t);
>>> /* We can get here with a decl that was synthesized by language-
>>
>> [...]
>>
>>> @@ -3598,20 +3628,20 @@ function_category (tree fn)
>>> && DECL_FUNCTION_MEMBER_P (fn))
>>> {
>>> if (DECL_STATIC_FUNCTION_P (fn))
>>> - return _("In static member function %qs");
>>> + return _("In static member function %qD");
>>> else if (DECL_COPY_CONSTRUCTOR_P (fn))
>>> - return _("In copy constructor %qs");
>>> + return _("In copy constructor %qD");
>>> else if (DECL_CONSTRUCTOR_P (fn))
>>> - return _("In constructor %qs");
>>> + return _("In constructor %qD");
>>> else if (DECL_DESTRUCTOR_P (fn))
>>> - return _("In destructor %qs");
>>> + return _("In destructor %qD");
>>> else if (LAMBDA_FUNCTION_P (fn))
>>> return _("In lambda function");
>>> else
>>> - return _("In member function %qs");
>>> + return _("In member function %qD");
>>> }
>>> else
>>> - return _("In function %qs");
>>> + return _("In function %qD");
>>> }
>>
>> The leading comment for function_category still refers to %qs and thus
>> needs updating.
>
> Fixed.
>
>>> @@ -4393,7 +4423,7 @@ cp_printer (pretty_printer *pp, text_info
>> *text, const char *spec,
>>> break;
>>> }
>>> }
>>> - result = decl_to_string (temp, verbose);
>>> + result = decl_to_string (temp, verbose, pp_show_color (pp));
>>> }
>>> break;
>>> case 'E': result = expr_to_string (next_tree); break;
>>
>> [...]
>>
>> FWIW there's no automated test coverage for this; there are some
>> examples of testing colorization in DejaGnu in the plugins
>> subdirectories e.g.
>> gcc/testsuite/g++.dg/plugin/show_template_tree_color_plugin.c
>> gcc/testsuite/g++.dg/plugin/show-template-tree-color.C
>> They can be awkward to write and maintain, but this stuff is non-
>> trivial, so it would be nice to have a test to make sure we don't
>> regress.
>>
>> To what extent have you been "eating your own dogfood" with this patch?
>> (i.e. having it applied to your primary compiler for development). I'm
>> a bit wary of making this change at this point in the release cycle
>> without spending some time "living with it" (and sorry again for not
>> commenting on it earlier; I was avoiding being online for most of
>> December)
>
> Not to a significant extent; this isn't so important for compiling
> relatively simple C++ code like we have in GCC, and I mostly build with
> the system compiler. I've mostly used it when debugging the compiler's
> handling of a larger testcase.
I left the patch in my working tree for much of stage 3/4, and just
reapplied it now because I missed it when trying to read error messages
from a broken testcase.
>> My feeling is that this patch would be OK with the above nits fixed,
>> given that it's relatively trivial to revert if it causes difficulties,
>> but we should test it "in anger".
>>
>> Hope this is constructive
>
> Definitely, thanks. Here's a revision to address those nits:
Here it is rebased. Any objection to applying it now?
From b6050ea38d01374e1c18809f4b5ff6f7d302a122 Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Fri, 18 Jun 2021 05:45:02 -0400
Subject: [PATCH] c++: add color to function decl printing
To: gcc-patches@gcc.gnu.org
In reading C++ diagnostics, it's often hard to find the name of the function
in the middle of the template header, return type, parameters, and template
arguments. So let's colorize it, and maybe the template argument bindings
while we're at it.
I've somewhat arbitrarily chosen bold green for the function name, and
non-bold magenta for the template arguments.
A side-effect of this is that when this happens in a quote (i.e. %qD), the
rest of the quote after the function name is no longer bold. I think that's
acceptable; returning to the bold would require maintaining a colorize stack
instead of the on/off controls we have now.
gcc/cp/ChangeLog:
* error.c (decl_to_string): Add show_color parameter.
(subst_to_string): Likewise.
(cp_printer): Pass it.
(type_to_string): Set pp_show_color.
(dump_function_name): Use "fnname" color.
(dump_template_bindings): Use "targs" color.
(struct colorize_guard): New.
(reinit_cxx_pp): Clear pp_show_color.
(cp_print_error_function): Use %qD.
(function_category): Use %qD.
gcc/ChangeLog:
* diagnostic-color.c: Add fnname and targs color entries.
* doc/invoke.texi: Document them.
---
gcc/doc/invoke.texi | 8 +++++
gcc/cp/error.c | 70 ++++++++++++++++++++++++++++++------------
gcc/diagnostic-color.c | 2 ++
3 files changed, 60 insertions(+), 20 deletions(-)
@@ -4803,6 +4803,14 @@ SGR substring for location information, @samp{file:line} or
@vindex quote GCC_COLORS @r{capability}
SGR substring for information printed within quotes.
+@item fnname=
+@vindex fnname GCC_COLORS @r{capability}
+SGR substring for names of C++ functions.
+
+@item targs=
+@vindex targs GCC_COLORS @r{capability}
+SGR substring for C++ function template parameter bindings.
+
@item fixit-insert=
@vindex fixit-insert GCC_COLORS @r{capability}
SGR substring for fix-it hints suggesting text to
@@ -1,4 +1,4 @@
-/* Call-backs for C++ error reporting.
+/* Call-backs for -*- C++ -*- error reporting.
This code is non-reentrant.
Copyright (C) 1993-2021 Free Software Foundation, Inc.
This file is part of GCC.
@@ -25,6 +25,7 @@ along with GCC; see the file COPYING3. If not see
#include "cp-tree.h"
#include "stringpool.h"
#include "tree-diagnostic.h"
+#include "diagnostic-color.h"
#include "langhooks-def.h"
#include "intl.h"
#include "cxx-pretty-print.h"
@@ -56,7 +57,7 @@ static cxx_pretty_printer * const cxx_pp = &actual_pretty_printer;
static const char *args_to_string (tree, int);
static const char *code_to_string (enum tree_code);
static const char *cv_to_string (tree, int);
-static const char *decl_to_string (tree, int);
+static const char *decl_to_string (tree, int, bool);
static const char *fndecl_to_string (tree, int);
static const char *op_to_string (bool, enum tree_code);
static const char *parm_to_string (int);
@@ -390,6 +391,7 @@ dump_template_bindings (cxx_pretty_printer *pp, tree parms, tree args,
else
{
pp_cxx_whitespace (pp);
+ pp_string (pp, colorize_start (pp_show_color (pp), "targs"));
pp_cxx_left_bracket (pp);
pp->translate_string ("with");
pp_cxx_whitespace (pp);
@@ -400,7 +402,10 @@ dump_template_bindings (cxx_pretty_printer *pp, tree parms, tree args,
~prepost_semicolon ()
{
if (need_semicolon)
- pp_cxx_right_bracket (pp);
+ {
+ pp_cxx_right_bracket (pp);
+ pp_string (pp, colorize_stop (pp_show_color (pp)));
+ }
}
} semicolon_or_introducer = {pp, false};
@@ -1158,6 +1163,22 @@ dump_simple_decl (cxx_pretty_printer *pp, tree t, tree type, int flags)
dump_type_suffix (pp, type, flags);
}
+struct colorize_guard
+{
+ bool colorize;
+ cxx_pretty_printer *pp;
+
+ colorize_guard (bool _colorize, cxx_pretty_printer *pp, const char *name)
+ : colorize (_colorize && pp_show_color (pp)), pp (pp)
+ {
+ pp_string (pp, colorize_start (colorize, name));
+ }
+ ~colorize_guard ()
+ {
+ pp_string (pp, colorize_stop (colorize));
+ }
+};
+
/* Print an IDENTIFIER_NODE that is the name of a declaration. */
static void
@@ -1928,6 +1949,11 @@ dump_exception_spec (cxx_pretty_printer *pp, tree t, int flags)
static void
dump_function_name (cxx_pretty_printer *pp, tree t, int flags)
{
+ bool colorize = flags & (TFF_DECL_SPECIFIERS | TFF_RETURN_TYPE
+ | TFF_TEMPLATE_HEADER);
+
+ colorize_guard g (colorize, pp, "fnname");
+
tree name = DECL_NAME (t);
/* We can get here with a decl that was synthesized by language-
@@ -3062,6 +3088,7 @@ reinit_cxx_pp (void)
cxx_pp->padding = pp_none;
pp_indentation (cxx_pp) = 0;
pp_needs_newline (cxx_pp) = false;
+ pp_show_color (cxx_pp) = false;
cxx_pp->enclosing_scope = current_function_decl;
}
@@ -3208,7 +3235,7 @@ location_of (tree t)
function. */
static const char *
-decl_to_string (tree decl, int verbose)
+decl_to_string (tree decl, int verbose, bool show_color)
{
int flags = 0;
@@ -3222,6 +3249,7 @@ decl_to_string (tree decl, int verbose)
flags |= TFF_TEMPLATE_HEADER;
reinit_cxx_pp ();
+ pp_show_color (cxx_pp) = show_color;
dump_decl (cxx_pp, decl, flags);
return pp_ggc_formatted_text (cxx_pp);
}
@@ -3321,6 +3349,7 @@ type_to_string (tree typ, int verbose, bool postprocessed, bool *quote,
flags |= TFF_TEMPLATE_HEADER;
reinit_cxx_pp ();
+ pp_show_color (cxx_pp) = show_color;
if (postprocessed && quote && *quote)
pp_begin_quote (cxx_pp, show_color);
@@ -3415,7 +3444,7 @@ args_to_string (tree p, int verbose)
arguments. */
static const char *
-subst_to_string (tree p)
+subst_to_string (tree p, bool show_color)
{
tree decl = TREE_PURPOSE (p);
tree targs = TREE_VALUE (p);
@@ -3427,6 +3456,7 @@ subst_to_string (tree p)
return "";
reinit_cxx_pp ();
+ pp_show_color (cxx_pp) = show_color;
dump_template_decl (cxx_pp, TREE_PURPOSE (p), flags);
dump_substitution (cxx_pp, NULL, tparms, targs, /*flags=*/0);
return pp_ggc_formatted_text (cxx_pp);
@@ -3517,7 +3547,7 @@ cp_print_error_function (diagnostic_context *context,
fndecl = current_function_decl;
pp_printf (context->printer, function_category (fndecl),
- cxx_printable_name_translate (fndecl, 2));
+ fndecl);
while (abstract_origin)
{
@@ -3561,19 +3591,19 @@ cp_print_error_function (diagnostic_context *context,
{
if (context->show_column && s.column != 0)
pp_printf (context->printer,
- _(" inlined from %qs at %r%s:%d:%d%R"),
- cxx_printable_name_translate (fndecl, 2),
+ _(" inlined from %qD at %r%s:%d:%d%R"),
+ fndecl,
"locus", s.file, s.line, s.column);
else
pp_printf (context->printer,
- _(" inlined from %qs at %r%s:%d%R"),
- cxx_printable_name_translate (fndecl, 2),
+ _(" inlined from %qD at %r%s:%d%R"),
+ fndecl,
"locus", s.file, s.line);
}
else
- pp_printf (context->printer, _(" inlined from %qs"),
- cxx_printable_name_translate (fndecl, 2));
+ pp_printf (context->printer, _(" inlined from %qD"),
+ fndecl);
}
}
pp_character (context->printer, ':');
@@ -3598,20 +3628,20 @@ function_category (tree fn)
&& DECL_FUNCTION_MEMBER_P (fn))
{
if (DECL_STATIC_FUNCTION_P (fn))
- return _("In static member function %qs");
+ return _("In static member function %qD");
else if (DECL_COPY_CONSTRUCTOR_P (fn))
- return _("In copy constructor %qs");
+ return _("In copy constructor %qD");
else if (DECL_CONSTRUCTOR_P (fn))
- return _("In constructor %qs");
+ return _("In constructor %qD");
else if (DECL_DESTRUCTOR_P (fn))
- return _("In destructor %qs");
+ return _("In destructor %qD");
else if (LAMBDA_FUNCTION_P (fn))
return _("In lambda function");
else
- return _("In member function %qs");
+ return _("In member function %qD");
}
else
- return _("In function %qs");
+ return _("In function %qD");
}
/* Disable warnings about missing quoting in GCC diagnostics for
@@ -4393,7 +4423,7 @@ cp_printer (pretty_printer *pp, text_info *text, const char *spec,
break;
}
}
- result = decl_to_string (temp, verbose);
+ result = decl_to_string (temp, verbose, pp_show_color (pp));
}
break;
case 'E': result = expr_to_string (next_tree); break;
@@ -4410,7 +4440,7 @@ cp_printer (pretty_printer *pp, text_info *text, const char *spec,
case 'O': result = op_to_string (false, next_tcode); break;
case 'P': result = parm_to_string (next_int); break;
case 'Q': result = op_to_string (true, next_tcode); break;
- case 'S': result = subst_to_string (next_tree); break;
+ case 'S': result = subst_to_string (next_tree, pp_show_color (pp)); break;
case 'T':
{
result = type_to_string (next_tree, verbose, false, quoted,
@@ -91,6 +91,8 @@ static struct color_cap color_dict[] =
{ "locus", SGR_SEQ (COLOR_BOLD), 5, false },
{ "quote", SGR_SEQ (COLOR_BOLD), 5, false },
{ "path", SGR_SEQ (COLOR_BOLD COLOR_SEPARATOR COLOR_FG_CYAN), 4, false },
+ { "fnname", SGR_SEQ (COLOR_BOLD COLOR_SEPARATOR COLOR_FG_GREEN), 6, false },
+ { "targs", SGR_SEQ (COLOR_FG_MAGENTA), 5, false },
{ "fixit-insert", SGR_SEQ (COLOR_FG_GREEN), 12, false },
{ "fixit-delete", SGR_SEQ (COLOR_FG_RED), 12, false },
{ "diff-filename", SGR_SEQ (COLOR_BOLD), 13, false },
--
2.27.0