* [PATCH RFC] c++: add color to function decl printing
@ 2021-12-12 5:39 Jason Merrill
2021-12-13 11:02 ` Jonathan Wakely
2021-12-13 19:22 ` [PATCH RFC] " Martin Sebor
0 siblings, 2 replies; 11+ messages in thread
From: Jason Merrill @ 2021-12-12 5:39 UTC (permalink / raw)
To: gcc-patches; +Cc: jwakely
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?
gcc/cp/ChangeLog:
* error.c (decl_to_string): Add show_color parameter.
(cp_printer): Pass it.
(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 | 64 ++++++++++++++++++++++++++++++------------
gcc/diagnostic-color.c | 2 ++
3 files changed, 56 insertions(+), 18 deletions(-)
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
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index daea3b39a15..e03079e3b8f 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 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);
}
@@ -3517,7 +3545,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 +3589,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 +3626,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 +4421,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;
diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
index 1fc5a9079c7..c1406b45d8b 100644
--- a/gcc/diagnostic-color.c
+++ b/gcc/diagnostic-color.c
@@ -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 },
base-commit: 2e8067041d1d69da02bd7578f58abc11eb35a04b
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC] c++: add color to function decl printing
2021-12-12 5:39 [PATCH RFC] c++: add color to function decl printing Jason Merrill
@ 2021-12-13 11:02 ` Jonathan Wakely
2021-12-13 14:58 ` [PATCH v2 " Jason Merrill
2021-12-13 19:22 ` [PATCH RFC] " Martin Sebor
1 sibling, 1 reply; 11+ messages in thread
From: Jonathan Wakely @ 2021-12-13 11:02 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc Patches
On Sun, 12 Dec 2021 at 05:39, Jason Merrill <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:19: error: no
match for call to '(std::ranges::__cust_access::_End) (adl::Footie [])'
/home/jwakely/gcc/12/include/c++/12.0.0/bits/ranges_base.h:165:9:
note: candidate:
'template<class _Tp> requires (__maybe_borrowed_range<_Tp>) &&
((is_bounded_array_v<typename std::remove_reference<_Tp>::type*>) ||
(__member_end<_Tp>) || (__adl_end<_Tp>)) constexpr auto
std::ranges::__cust_access::_End::operator()(_Tp&&) const' *
165 | operator()[[nodiscard]](_Tp&& __t) const
noexcept(_S_noexcept<_Tp&>())
| ^~~~~~~~
/home/jwakely/gcc/12/include/c++/12.0.0/bits/ranges_base.h:165:9:
note: template
argument deduction/substitution failed:
/home/jwakely/gcc/12/include/c++/12.0.0/bits/ranges_base.h:165:9:
note: constraints
not satisfied
/home/jwakely/gcc/12/include/c++/12.0.0/bits/ranges_base.h: In substitution
of 'template<class _Tp> requires (__maybe_borrowed_range<_Tp>) &&
((is_bounded_array_v<typename std::remove_reference<_Tp>::type>) |*|
(__member_end<_Tp>) || (__adl_end<_Tp>)) constexpr auto
std::ranges::__cust_access::_End::operator()(_Tp&&) const [with _Tp =
adl::Footie (&)[]]*':
/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 (&)[]]*'
r.C:19:27: required from here
/home/jwakely/gcc/12/include/c++/12.0.0/bits/ranges_base.h:165:2:
required by the constraints of 'template<class _Tp> requires
(__maybe_borrowed_range<_Tp>) && ((is_bounded_array_v<typename
std::remove_refe*rence<_Tp>::type>) || (__member_end<_Tp>) ||
(__adl_end<_Tp>)) constexpr auto
std::ranges::__cust_access::_End::operator()**(_Tp&&) const*'
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()".
With Konsole's Solarized (dark) colour scheme the FG_GREEN colour is
actually a slightly darker grey than the surrounding text, which makes it
harder to find, but I can adjust that with GCC_COLORS.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 RFC] c++: add color to function decl printing
2021-12-13 11:02 ` Jonathan Wakely
@ 2021-12-13 14:58 ` Jason Merrill
2022-01-14 21:49 ` David Malcolm
0 siblings, 1 reply; 11+ messages in thread
From: Jason Merrill @ 2021-12-13 14:58 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: gcc Patches
[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]
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
[-- Attachment #2: 0001-c-add-color-to-function-decl-printing.patch --]
[-- Type: text/x-patch, Size: 10016 bytes --]
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(-)
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
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 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,
diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
index 1fc5a9079c7..c1406b45d8b 100644
--- a/gcc/diagnostic-color.c
+++ b/gcc/diagnostic-color.c
@@ -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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC] c++: add color to function decl printing
2021-12-12 5:39 [PATCH RFC] c++: add color to function decl printing Jason Merrill
2021-12-13 11:02 ` Jonathan Wakely
@ 2021-12-13 19:22 ` Martin Sebor
2021-12-13 23:00 ` Jonathan Wakely
2021-12-14 5:41 ` Jason Merrill
1 sibling, 2 replies; 11+ messages in thread
From: Martin Sebor @ 2021-12-13 19:22 UTC (permalink / raw)
To: Jason Merrill, gcc-patches; +Cc: jwakely
On 12/11/21 10:39 PM, Jason Merrill via Gcc-patches 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 appreciate the problem but I can't say I find this solution
much of an improvement. We end up with the same name in up to
four colors: cyan, magenta, green, and black, plus bold versions
of each, depending on where in the text the name appears. It's
not apparent to me what the different colors mean or how they
help.
IMO, the underlying root cause for why relevant details are so
hard to find in G++ messages is that there's so much redundancy
and irrelevant context in the output. For instance, for this
test case:
#include <map>
std::map<const char*, const char*> m ("123", "456");
GCC produces 10 screenfuls of output (more than 10 times as many
as Clang). GCC produces so much more output because it repeats
the full set of included files before each candidate (even though
the headers are the same in each), and also because it repeats
the full set of template arguments each time. E.g., like so:
In file included from
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:64,
from
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_tree.h:63,
from
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/map:60,
from t.C:1:
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_pair.h:558:28:
note: candidate: ‘template<class _U1, class _U2, typename
std::enable_if<(std::_PCC<((! std::is_same<const char* const,
_U1>::value) || (! std::is_same<const char*, _U2>::value)), const char*
const, const char*>::_MoveConstructiblePair<_U1, _U2>() && (!
std::_PCC<((! std::is_same<const char* const, _U1>::value) || (!
std::is_same<const char*, _U2>::value)), const char* const, const
char*>::_ImplicitlyMoveConvertiblePair<_U1, _U2>())), bool>::type
<anonymous> > constexpr std::pair<_T1, _T2>::pair(std::pair<_U1, _U2>&&)
[with _U2 = _U1; typename std::enable_if<(std::_PCC<((!
std::is_same<_T1, _U1>::value) || (! std::is_same<_T2, _U2>::value)),
_T1, _T2>::_MoveConstructiblePair<_U1, _U2>() && (! std::_PCC<((!
std::is_same<_T1, _U1>::value) || (! std::is_same<_T2, _U2>::value)),
_T1, _T2>::_ImplicitlyMoveConvertiblePair<_U1, _U2>())), bool>::type
<anonymous> = _U2; _T1 = const char* const; _T2 = const char*]’
558 | explicit constexpr pair(pair<_U1, _U2>&& __p)
| ^~~~
I suspect focusing on reducing the amount of superfluous output
would be a better approach than colorizing the precious nuggets
of useful information in it.
Martin
PS Years ago (in the early 2000's) when interviewing experienced
candidates for C++ template library positions, we'd sit them in
front of a machine with a few C++ compilers and the test case
above. They were asked to find and explain and fix the problem
in the test case, using any or all of the compilers. Of all
the people we interviewed only one guy was able to decipher
the errors enough to find the bug. Looks like this could still
be a good exercise (though we might have to keep them from using
Clang ;)
>
> gcc/cp/ChangeLog:
>
> * error.c (decl_to_string): Add show_color parameter.
> (cp_printer): Pass it.
> (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 | 64 ++++++++++++++++++++++++++++++------------
> gcc/diagnostic-color.c | 2 ++
> 3 files changed, 56 insertions(+), 18 deletions(-)
>
> 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
> diff --git a/gcc/cp/error.c b/gcc/cp/error.c
> index daea3b39a15..e03079e3b8f 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 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);
> }
> @@ -3517,7 +3545,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 +3589,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 +3626,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 +4421,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;
> diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
> index 1fc5a9079c7..c1406b45d8b 100644
> --- a/gcc/diagnostic-color.c
> +++ b/gcc/diagnostic-color.c
> @@ -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 },
>
> base-commit: 2e8067041d1d69da02bd7578f58abc11eb35a04b
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC] c++: add color to function decl printing
2021-12-13 19:22 ` [PATCH RFC] " Martin Sebor
@ 2021-12-13 23:00 ` Jonathan Wakely
2021-12-14 5:41 ` Jason Merrill
1 sibling, 0 replies; 11+ messages in thread
From: Jonathan Wakely @ 2021-12-13 23:00 UTC (permalink / raw)
To: Martin Sebor; +Cc: Jason Merrill, gcc Patches
On Mon, 13 Dec 2021 at 19:22, Martin Sebor <msebor@gmail.com> wrote:
> On 12/11/21 10:39 PM, Jason Merrill via Gcc-patches 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 appreciate the problem but I can't say I find this solution
> much of an improvement. We end up with the same name in up to
> four colors: cyan, magenta, green, and black, plus bold versions
> of each, depending on where in the text the name appears. It's
> not apparent to me what the different colors mean or how they
> help.
>
They break up the wall of text. You don't have to know what they mean to be
able to see where one chunk of text ends and another begins (in a different
colour). The colours don't *mean* anything, they're just a way to
distinguish different logical parts of the output.
> IMO, the underlying root cause for why relevant details are so
> hard to find in G++ messages is that there's so much redundancy
> and irrelevant context in the output. For instance, for this
> test case:
>
> #include <map>
>
> std::map<const char*, const char*> m ("123", "456");
>
>
For this example, I think putting the [with T = ...; U = ...] template
arguments in a different colours helps a lot. It delineates the end of the
signature and the start of the template args, which helps navigate the wall
of text.
I don't think anybody can argue that it's easier when the [with T = ...]
part is just a continuation of the text before it. Making it a different
colour doesn't necessarily make the errors easier to understand if you're
not familiar with the code or GCC's diagnostic format, but I definitely
think it makes it easier to navigate. And that should make it easier to
figure out what the error is saying.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC] c++: add color to function decl printing
2021-12-13 19:22 ` [PATCH RFC] " Martin Sebor
2021-12-13 23:00 ` Jonathan Wakely
@ 2021-12-14 5:41 ` Jason Merrill
2021-12-14 18:02 ` Martin Sebor
1 sibling, 1 reply; 11+ messages in thread
From: Jason Merrill @ 2021-12-14 5:41 UTC (permalink / raw)
To: Martin Sebor, gcc-patches; +Cc: jwakely
On 12/13/21 14:22, Martin Sebor wrote:
> On 12/11/21 10:39 PM, Jason Merrill via Gcc-patches 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 appreciate the problem but I can't say I find this solution
> much of an improvement. We end up with the same name in up to
> four colors: cyan, magenta, green, and black, plus bold versions
> of each, depending on where in the text the name appears. It's
> not apparent to me what the different colors mean or how they
> help.
You can get the same name in different colors because the diagnostic is
telling you something different about it, if it's e.g. the name of a
function we're printing or the source text being indicated as the source
of the problem. Is it really unclear what the different colors mean? I
find it much easier to read the output for your testcase after this
patch, as highlighting the function name and lowlighting the template
args means that
map(_InputIterator, _InputIterator)
stands out as the problematic function.
> IMO, the underlying root cause for why relevant details are so
> hard to find in G++ messages is that there's so much redundancy
> and irrelevant context in the output. For instance, for this
> test case:
>
> #include <map>
>
> std::map<const char*, const char*> m ("123", "456");
>
> GCC produces 10 screenfuls of output (more than 10 times as many
> as Clang). GCC produces so much more output because it repeats
> the full set of included files before each candidate (even though
> the headers are the same in each),
Yes, unfortunately the explanation of why each candidate is non-viable
switches files. Perhaps we should remember files that we've already
listed the include path for and avoid repeating it.
> and also because it repeats
> the full set of template arguments each time. E.g., like so:
> In file included from
> /build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:64,
>
> from
> /build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_tree.h:63,
>
> from
> /build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/map:60,
> from t.C:1:
> /build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_pair.h:558:28:
> note: candidate: ‘template<class _U1, class _U2, typename
> std::enable_if<(std::_PCC<((! std::is_same<const char* const,
> _U1>::value) || (! std::is_same<const char*, _U2>::value)), const char*
> const, const char*>::_MoveConstructiblePair<_U1, _U2>() && (!
> std::_PCC<((! std::is_same<const char* const, _U1>::value) || (!
> std::is_same<const char*, _U2>::value)), const char* const, const
> char*>::_ImplicitlyMoveConvertiblePair<_U1, _U2>())), bool>::type
> <anonymous> > constexpr std::pair<_T1, _T2>::pair(std::pair<_U1, _U2>&&)
> [with _U2 = _U1; typename std::enable_if<(std::_PCC<((!
> std::is_same<_T1, _U1>::value) || (! std::is_same<_T2, _U2>::value)),
> _T1, _T2>::_MoveConstructiblePair<_U1, _U2>() && (! std::_PCC<((!
> std::is_same<_T1, _U1>::value) || (! std::is_same<_T2, _U2>::value)),
> _T1, _T2>::_ImplicitlyMoveConvertiblePair<_U1, _U2>())), bool>::type
> <anonymous> = _U2; _T1 = const char* const; _T2 = const char*]’
> 558 | explicit constexpr pair(pair<_U1, _U2>&& __p)
> | ^~~~
>
> I suspect focusing on reducing the amount of superfluous output
> would be a better approach than colorizing the precious nuggets
> of useful information in it.
Why not both?
I've experimented with making candidate printing in particular less
verbose; I expect the return type is rarely interesting in a call context.
> PS Years ago (in the early 2000's) when interviewing experienced
> candidates for C++ template library positions, we'd sit them in
> front of a machine with a few C++ compilers and the test case
> above. They were asked to find and explain and fix the problem
> in the test case, using any or all of the compilers. Of all
> the people we interviewed only one guy was able to decipher
> the errors enough to find the bug. Looks like this could still
> be a good exercise (though we might have to keep them from using
> Clang ;)
Note that the clang output leaves out the crucial information that the
constructor selected is the one that takes iterators.
>> gcc/cp/ChangeLog:
>>
>> * error.c (decl_to_string): Add show_color parameter.
>> (cp_printer): Pass it.
>> (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 | 64 ++++++++++++++++++++++++++++++------------
>> gcc/diagnostic-color.c | 2 ++
>> 3 files changed, 56 insertions(+), 18 deletions(-)
>>
>> 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
>> diff --git a/gcc/cp/error.c b/gcc/cp/error.c
>> index daea3b39a15..e03079e3b8f 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 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);
>> }
>> @@ -3517,7 +3545,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 +3589,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 +3626,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 +4421,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;
>> diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
>> index 1fc5a9079c7..c1406b45d8b 100644
>> --- a/gcc/diagnostic-color.c
>> +++ b/gcc/diagnostic-color.c
>> @@ -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 },
>>
>> base-commit: 2e8067041d1d69da02bd7578f58abc11eb35a04b
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC] c++: add color to function decl printing
2021-12-14 5:41 ` Jason Merrill
@ 2021-12-14 18:02 ` Martin Sebor
0 siblings, 0 replies; 11+ messages in thread
From: Martin Sebor @ 2021-12-14 18:02 UTC (permalink / raw)
To: Jason Merrill, gcc-patches; +Cc: jwakely
[-- Attachment #1: Type: text/plain, Size: 18607 bytes --]
On 12/13/21 10:41 PM, Jason Merrill wrote:
> On 12/13/21 14:22, Martin Sebor wrote:
>> On 12/11/21 10:39 PM, Jason Merrill via Gcc-patches 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 appreciate the problem but I can't say I find this solution
>> much of an improvement. We end up with the same name in up to
>> four colors: cyan, magenta, green, and black, plus bold versions
>> of each, depending on where in the text the name appears. It's
>> not apparent to me what the different colors mean or how they
>> help.
>
> You can get the same name in different colors because the diagnostic is
> telling you something different about it, if it's e.g. the name of a
> function we're printing or the source text being indicated as the source
> of the problem. Is it really unclear what the different colors mean? I
> find it much easier to read the output for your testcase after this
> patch, as highlighting the function name and lowlighting the template
> args means that
>
> map(_InputIterator, _InputIterator)
>
> stands out as the problematic function.
I understand why you want to draw attention to some parts of
the message and I think that could be useful if done without
relying on color as the sole attribute (think of users of
monochrome or low color terminals or the color-blind), with
more consistency, and without "overloading" existing colors
to mean something subtly different in different contexts (green
is already used for insertion hints, and magenta for warnings).
Given a simple case like this:
struct A
{
A (T, T);
};
A<int> a (1.0);
t.C:7:14: error: no matching function for call to ‘A<int>::A(double)’
7 | A<int> a (1.0);
| ^
t.C:4:3: note: candidate: ‘A<T>::A(T, T) [with T = int]’
4 | A (T, T);
| ^
t.C:4:3: note: candidate expects 2 arguments, 1 provided
t.C:2:8: note: candidate: ‘constexpr A<int>::A(const A<int>&)’
2 | struct A
| ^
t.C:2:8: note: no known conversion for argument 1 from ‘double’ to
‘const A<int>&’
t.C:2:8: note: candidate: ‘constexpr A<int>::A(A<int>&&)’
t.C:2:8: note: no known conversion for argument 1 from ‘double’ to
‘A<int>&&’
In the attached screenshot of the output it's reasonably clear
that the green A in the messages names the candidate function.
But even there the function's name is rendered in three colors:
black in the error message, green in the notes, and cyan in
the source code quoted in the notes. What is not clear is
why. (I can guess that it's simply an artifact of how the GCC
diagnostic machinery works, but that's neither intuitive to
users nor helpful.)
But simple cases are clear even with no colors. What you'd like
to do is improve the not-so-simple cases like the one with
std::map and that's where the color choices become much less
clear: in long messages with lots of the same names highlighted
in different colors. It seems especially unhelpful that some
of the text in the same color is bold while other such text is
not (the [with T = ...] parts). Using the same (or very similar)
colors for entirely different things (warnings and fix-it hints)
compounds the problem.
>
>> IMO, the underlying root cause for why relevant details are so
>> hard to find in G++ messages is that there's so much redundancy
>> and irrelevant context in the output. For instance, for this
>> test case:
>>
>> #include <map>
>>
>> std::map<const char*, const char*> m ("123", "456");
>>
>> GCC produces 10 screenfuls of output (more than 10 times as many
>> as Clang). GCC produces so much more output because it repeats
>> the full set of included files before each candidate (even though
>> the headers are the same in each),
>
> Yes, unfortunately the explanation of why each candidate is non-viable
> switches files. Perhaps we should remember files that we've already
> listed the include path for and avoid repeating it.
I was thinking the same thing. Paths to system headers could
also be abbreviated (or common prefixes replaced by some symbol).
Perhaps even the repetitive [with T = ...] could be removed in
subsequent messages to reduce the clutter (I notice Clang does
away with either all or most of it altogether in some messages
involving standard containers like std::string or std::vector).
>
>> and also because it repeats
>> the full set of template arguments each time. E.g., like so:
>> In file included from
>> /build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:64,
>>
>> from
>> /build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_tree.h:63,
>>
>> from
>> /build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/map:60,
>> from t.C:1:
>> /build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_pair.h:558:28:
>> note: candidate: ‘template<class _U1, class _U2, typename
>> std::enable_if<(std::_PCC<((! std::is_same<const char* const,
>> _U1>::value) || (! std::is_same<const char*, _U2>::value)), const
>> char* const, const char*>::_MoveConstructiblePair<_U1, _U2>() && (!
>> std::_PCC<((! std::is_same<const char* const, _U1>::value) || (!
>> std::is_same<const char*, _U2>::value)), const char* const, const
>> char*>::_ImplicitlyMoveConvertiblePair<_U1, _U2>())), bool>::type
>> <anonymous> > constexpr std::pair<_T1, _T2>::pair(std::pair<_U1,
>> _U2>&&) [with _U2 = _U1; typename std::enable_if<(std::_PCC<((!
>> std::is_same<_T1, _U1>::value) || (! std::is_same<_T2, _U2>::value)),
>> _T1, _T2>::_MoveConstructiblePair<_U1, _U2>() && (! std::_PCC<((!
>> std::is_same<_T1, _U1>::value) || (! std::is_same<_T2, _U2>::value)),
>> _T1, _T2>::_ImplicitlyMoveConvertiblePair<_U1, _U2>())), bool>::type
>> <anonymous> = _U2; _T1 = const char* const; _T2 = const char*]’
>> 558 | explicit constexpr pair(pair<_U1, _U2>&& __p)
>> | ^~~~
>>
>> I suspect focusing on reducing the amount of superfluous output
>> would be a better approach than colorizing the precious nuggets
>> of useful information in it.
>
> Why not both?
Both would be preferable to relying solely on colors. It's one
of the main points taught by user interface design guidelines.
My overall impression from my IDE development days is that less
is more.
>
> I've experimented with making candidate printing in particular less
> verbose; I expect the return type is rarely interesting in a call context.
I think that would be a better starting point. I believe
the biggest win lies in reducing the amount of verbosity (noise
and redundancy) in these messages. If we pare it down enough,
highlighting may just become an optional nice-to-have feature
rather than a necessity. There are also other ways to improve
readability, like formatting the messages to better fit
the screen width and indenting text or lining up like components
(I think the EDG front end beta does something like that).
Martin
>
>> PS Years ago (in the early 2000's) when interviewing experienced
>> candidates for C++ template library positions, we'd sit them in
>> front of a machine with a few C++ compilers and the test case
>> above. They were asked to find and explain and fix the problem
>> in the test case, using any or all of the compilers. Of all
>> the people we interviewed only one guy was able to decipher
>> the errors enough to find the bug. Looks like this could still
>> be a good exercise (though we might have to keep them from using
>> Clang ;)
>
> Note that the clang output leaves out the crucial information that the
> constructor selected is the one that takes iterators.
>
>>> gcc/cp/ChangeLog:
>>>
>>> * error.c (decl_to_string): Add show_color parameter.
>>> (cp_printer): Pass it.
>>> (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 | 64 ++++++++++++++++++++++++++++++------------
>>> gcc/diagnostic-color.c | 2 ++
>>> 3 files changed, 56 insertions(+), 18 deletions(-)
>>>
>>> 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
>>> diff --git a/gcc/cp/error.c b/gcc/cp/error.c
>>> index daea3b39a15..e03079e3b8f 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 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);
>>> }
>>> @@ -3517,7 +3545,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 +3589,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 +3626,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 +4421,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;
>>> diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
>>> index 1fc5a9079c7..c1406b45d8b 100644
>>> --- a/gcc/diagnostic-color.c
>>> +++ b/gcc/diagnostic-color.c
>>> @@ -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 },
>>>
>>> base-commit: 2e8067041d1d69da02bd7578f58abc11eb35a04b
>>>
>>
>
[-- Attachment #2: simple.png --]
[-- Type: image/png, Size: 63982 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 RFC] c++: add color to function decl printing
2021-12-13 14:58 ` [PATCH v2 " Jason Merrill
@ 2022-01-14 21:49 ` David Malcolm
2022-01-14 22:34 ` Jonathan Wakely
2022-01-15 3:56 ` [PATCH v3 " Jason Merrill
0 siblings, 2 replies; 11+ messages in thread
From: David Malcolm @ 2022-01-14 21:49 UTC (permalink / raw)
To: Jason Merrill, Jonathan Wakely; +Cc: gcc Patches
[-- Attachment #1: Type: text/plain, Size: 8230 bytes --]
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
[-- Attachment #2: Screenshot from 2022-01-14 15-55-18.png --]
[-- Type: image/png, Size: 137050 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 RFC] c++: add color to function decl printing
2022-01-14 21:49 ` David Malcolm
@ 2022-01-14 22:34 ` Jonathan Wakely
2022-01-15 3:56 ` [PATCH v3 " Jason Merrill
1 sibling, 0 replies; 11+ messages in thread
From: Jonathan Wakely @ 2022-01-14 22:34 UTC (permalink / raw)
To: David Malcolm; +Cc: Jason Merrill, gcc Patches
On Fri, 14 Jan 2022 at 21:49, David Malcolm wrote:
>
> Jonathan, did you try the v2 patch?
>
No, sorry.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 RFC] c++: add color to function decl printing
2022-01-14 21:49 ` David Malcolm
2022-01-14 22:34 ` Jonathan Wakely
@ 2022-01-15 3:56 ` Jason Merrill
2022-05-04 18:46 ` [PATCH v4] " Jason Merrill
1 sibling, 1 reply; 11+ messages in thread
From: Jason Merrill @ 2022-01-15 3:56 UTC (permalink / raw)
To: David Malcolm, Jonathan Wakely; +Cc: gcc Patches
[-- Attachment #1: Type: text/plain, Size: 9361 bytes --]
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:
[-- Attachment #2: 0001-c-add-color-to-function-decl-printing.patch --]
[-- Type: text/x-patch, Size: 11866 bytes --]
From 2c6256f4a5094b3caaff3f05d61dedeb2609d417 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/testsuite/ChangeLog:
* g++.dg/diagnostic/function-color1.C: New test.
---
gcc/doc/invoke.texi | 10 ++-
gcc/cp/error.c | 73 ++++++++++++++-----
gcc/diagnostic-color.c | 2 +
.../g++.dg/diagnostic/function-color1.C | 21 ++++++
4 files changed, 85 insertions(+), 21 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/diagnostic/function-color1.C
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 5504971ea81..cd073c3c37e 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -4758,7 +4758,7 @@ The default @env{GCC_COLORS} is
error=01;31:warning=01;35:note=01;36:range1=32:range2=34:locus=01:\
quote=01:path=01;36:fixit-insert=32:fixit-delete=31:\
diff-filename=01:diff-hunk=32:diff-delete=31:diff-insert=32:\
-type-diff=01;32
+type-diff=01;32:fnname=01;32:targs=35
@end smallexample
@noindent
where @samp{01;31} is bold red, @samp{01;35} is bold magenta,
@@ -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
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 8a3b7b5537c..4c29596c304 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -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);
}
+class colorize_guard
+{
+ bool colorize;
+ cxx_pretty_printer *pp;
+public:
+ 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,13 @@ dump_exception_spec (cxx_pretty_printer *pp, tree t, int flags)
static void
dump_function_name (cxx_pretty_printer *pp, tree t, int flags)
{
+ /* Only colorize when we're printing something before the name; in
+ particular, not when printing a CALL_EXPR. */
+ 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 +3090,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 +3237,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 +3251,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 +3351,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 +3446,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 +3458,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 +3549,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 +3593,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, ':');
@@ -3587,7 +3619,8 @@ cp_print_error_function (diagnostic_context *context,
}
/* Returns a description of FUNCTION using standard terminology. The
- result is a format string of the form "In CATEGORY %qs". */
+ result is a format string of the form "In CATEGORY %qD". */
+
static const char *
function_category (tree fn)
{
@@ -3598,20 +3631,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 +4426,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 +4443,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,
diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
index fa30707975f..f0baab2a282 100644
--- a/gcc/diagnostic-color.c
+++ b/gcc/diagnostic-color.c
@@ -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 },
diff --git a/gcc/testsuite/g++.dg/diagnostic/function-color1.C b/gcc/testsuite/g++.dg/diagnostic/function-color1.C
new file mode 100644
index 00000000000..32d9e966bdb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/function-color1.C
@@ -0,0 +1,21 @@
+// Verify colorization of printing of function declarations.
+// Use dg-*-multiline-output to avoid regexp interpretation.
+
+// { dg-options "-fdiagnostics-color=always" }
+
+template <class T> void f(short t);
+template <class T> void f(long t);
+
+int main()
+{
+ f<int>(42);
+ /* { dg-begin-multiline-output "" }
+call of overloaded '^[[01m^[[Kf<int>(int)^[[m^[[K' is ambiguous
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+candidate: '^[[01m^[[Kvoid^[[01;32m^[[K f^[[m^[[K(short int) ^[[35m^[[K[with T = int]^[[m^[[K^[[m^[[K'
+ { dg-end-multiline-output "" } */
+}
+
+// Discard the remaining colorized output that confuses dejagnu.
+// { dg-prune-output diagnostic/function-color1.C }
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] c++: add color to function decl printing
2022-01-15 3:56 ` [PATCH v3 " Jason Merrill
@ 2022-05-04 18:46 ` Jason Merrill
0 siblings, 0 replies; 11+ messages in thread
From: Jason Merrill @ 2022-05-04 18:46 UTC (permalink / raw)
To: David Malcolm, Jonathan Wakely; +Cc: gcc Patches
[-- Attachment #1: Type: text/plain, Size: 10195 bytes --]
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?
[-- Attachment #2: 0001-c-add-color-to-function-decl-printing.patch --]
[-- Type: text/x-patch, Size: 9404 bytes --]
From 9802b7ea9387e572ecd448fb6c09e82764985326 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.
gcc/ChangeLog:
* diagnostic-color.c: Add fnname and targs color entries.
* doc/invoke.texi: Document them.
gcc/testsuite/ChangeLog:
* g++.dg/diagnostic/function-color1.C: New test.
---
gcc/doc/invoke.texi | 10 ++++-
gcc/cp/error.cc | 44 ++++++++++++++++---
gcc/diagnostic-color.cc | 2 +
.../g++.dg/diagnostic/function-color1.C | 21 +++++++++
4 files changed, 70 insertions(+), 7 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/diagnostic/function-color1.C
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 3f4d6f2db7b..7a35d9613a4 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -4845,7 +4845,7 @@ The default @env{GCC_COLORS} is
error=01;31:warning=01;35:note=01;36:range1=32:range2=34:locus=01:\
quote=01:path=01;36:fixit-insert=32:fixit-delete=31:\
diff-filename=01:diff-hunk=32:diff-delete=31:diff-insert=32:\
-type-diff=01;32
+type-diff=01;32:fnname=01;32:targs=35
@end smallexample
@noindent
where @samp{01;31} is bold red, @samp{01;35} is bold magenta,
@@ -4890,6 +4890,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
diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
index 2e9dd2c6abd..250e012c008 100644
--- a/gcc/cp/error.cc
+++ b/gcc/cp/error.cc
@@ -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};
@@ -1170,6 +1175,22 @@ dump_simple_decl (cxx_pretty_printer *pp, tree t, tree type, int flags)
dump_type_suffix (pp, type, flags);
}
+class colorize_guard
+{
+ bool colorize;
+ cxx_pretty_printer *pp;
+public:
+ 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
@@ -1946,6 +1967,13 @@ dump_exception_spec (cxx_pretty_printer *pp, tree t, int flags)
static void
dump_function_name (cxx_pretty_printer *pp, tree t, int flags)
{
+ /* Only colorize when we're printing something before the name; in
+ particular, not when printing a CALL_EXPR. */
+ 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-
@@ -3088,6 +3116,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;
}
@@ -3234,7 +3263,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;
@@ -3248,6 +3277,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);
}
@@ -3347,6 +3377,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);
@@ -3441,7 +3472,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);
@@ -3453,6 +3484,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);
@@ -4420,7 +4452,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;
@@ -4437,7 +4469,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,
diff --git a/gcc/diagnostic-color.cc b/gcc/diagnostic-color.cc
index 640adfad558..95047d78c2e 100644
--- a/gcc/diagnostic-color.cc
+++ b/gcc/diagnostic-color.cc
@@ -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 },
diff --git a/gcc/testsuite/g++.dg/diagnostic/function-color1.C b/gcc/testsuite/g++.dg/diagnostic/function-color1.C
new file mode 100644
index 00000000000..32d9e966bdb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/function-color1.C
@@ -0,0 +1,21 @@
+// Verify colorization of printing of function declarations.
+// Use dg-*-multiline-output to avoid regexp interpretation.
+
+// { dg-options "-fdiagnostics-color=always" }
+
+template <class T> void f(short t);
+template <class T> void f(long t);
+
+int main()
+{
+ f<int>(42);
+ /* { dg-begin-multiline-output "" }
+call of overloaded '^[[01m^[[Kf<int>(int)^[[m^[[K' is ambiguous
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+candidate: '^[[01m^[[Kvoid^[[01;32m^[[K f^[[m^[[K(short int) ^[[35m^[[K[with T = int]^[[m^[[K^[[m^[[K'
+ { dg-end-multiline-output "" } */
+}
+
+// Discard the remaining colorized output that confuses dejagnu.
+// { dg-prune-output diagnostic/function-color1.C }
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-05-04 18:46 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-12 5:39 [PATCH RFC] c++: add color to function decl printing Jason Merrill
2021-12-13 11:02 ` Jonathan Wakely
2021-12-13 14:58 ` [PATCH v2 " Jason Merrill
2022-01-14 21:49 ` David Malcolm
2022-01-14 22:34 ` Jonathan Wakely
2022-01-15 3:56 ` [PATCH v3 " Jason Merrill
2022-05-04 18:46 ` [PATCH v4] " Jason Merrill
2021-12-13 19:22 ` [PATCH RFC] " Martin Sebor
2021-12-13 23:00 ` Jonathan Wakely
2021-12-14 5:41 ` Jason Merrill
2021-12-14 18:02 ` Martin Sebor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).