This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: RFC: color diagnostics markers
- From: Manuel López-Ibáñez <lopezibanez at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Gabriel Dos Reis <gdr at integrable-solutions dot net>, "Joseph S. Myers" <joseph at codesourcery dot com>, Gcc Patch List <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 8 Apr 2013 19:54:18 +0200
- Subject: Re: RFC: color diagnostics markers
- References: <CAESRpQCG6VX4JTEXLhrV2LDCLENYB7i8ad-3jj=K6FGeuK5+Ug at mail dot gmail dot com> <20130402091449 dot GE20616 at tucnak dot redhat dot com> <CAESRpQAiDrn-9T7Zxvs2ZQVT9hE9_X=gnEyDkS7Y4b-vOiz3Cg at mail dot gmail dot com> <20130408132301 dot GO20334 at tucnak dot redhat dot com> <CAESRpQAz45k2SYVcEDn_oVkUuLVtGqChHRdgS0NQW5dVgrveLw at mail dot gmail dot com> <20130408144359 dot GP20334 at tucnak dot redhat dot com>
On 8 April 2013 16:43, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Apr 08, 2013 at 04:29:02PM +0200, Manuel López-Ibáñez wrote:
>> In fact, I would be fine with something like:
>>
>> pp_start_color()
>> pp_stop_color()
>> pp_wrap_in_color("")
>>
>> It is a bit more verbose, but also clearer when reading the code. And
>> no need for %[colorname] or %r or -Wformat support.
>
> But you then need to break the code into multiple function calls, which
> decreases readability.
>
> pp_verbatim (context->printer,
> _("%s:%d:%d: [ skipping %d instantiation contexts, "
> "use -ftemplate-backtrace-limit=0 to disable ]\n"),
> xloc.file, xloc.line, xloc.column, skip);
I guess "decreases readability" depends whether one knows what the
extra codes mean or not. I still have to check many times what %K and
%q#+T and other less common codes exactly do. I'd rather have less
codes than more.
And one could argue that the above call should be split, since the
"%s:%d:%d:" should not be translated.
That said, I would prefer that instead of
expanded_location xloc;
xloc = expand_location (loc);
if (context->show_column)
pp_verbatim (context->printer,
_("%r%s:%d:%d:%R [ skipping %d instantiation
contexts, "
"use -ftemplate-backtrace-limit=0 to disable ]\n"),
"locus", xloc.file, xloc.line, xloc.column, skip);
else
pp_verbatim (context->printer,
_("%r%s:%d:%R [ skipping %d instantiation contexts, "
"use -ftemplate-backtrace-limit=0 to disable ]\n"),
"locus", xloc.file, xloc.line, skip);
we had:
pp_verbatim (context->printer,
_("%X [ skipping %d instantiation contexts, "
"use -ftemplate-backtrace-limit=0 to disable ]\n"),
expand_location(loc), skip);
and the pretty-printer takes care of applying color or not (or
expanding column numbers or not, etc).
Or without the extra %X code:
pp_print_locus (context->printer, loc);
pp_verbatim (context->printer,
_(" [ skipping %d instantiation contexts, "
"use -ftemplate-backtrace-limit=0 to
disable ]\n"), skip);
Or even if we don't want the pretty printer to know about ->show_column:
diagnostic_print_locus (context, loc);
pp_verbatim (context->printer,
_(" [ skipping %d instantiation contexts, "
"use -ftemplate-backtrace-limit=0 to
disable ]\n"), skip);
and the internal diagnostics machinery takes care of applying color or
not (or expanding column numbers or not, etc).
> can be right now a single call, while you would need several. Also, if you
> eventually want to colorize something in say error_at, warning_at and
> similar format strings. For those you really don't have the printer at
Do we really want to allow that much flexibility? Then the color_dict
needs to be dynamic or the caller is restricted to re-using existing
colornames.
I was expecting the use of color to be rather limited to a very very
few well-defined concepts. I was hoping that higher-level diagnostic
functions would be oblivious to the color stuff to not make the
diagnostics code much more complex.
Maybe I am wrong here and FE maintainers do want that flexibility.
Cheers,
Manuel.