| Message ID | 20211212053942.240160-1-jason@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] c++: add color to function decl printing | expand |
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.
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 >
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.
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 >> >
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 >>> >> >
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 },