On 8 April 2013 15:23, Jakub Jelinek <ja...@redhat.com> wrote:
> On Fri, Apr 05, 2013 at 11:51:43PM +0200, Manuel López-Ibáñez wrote:
>> In this patch the default is "never", because for some reason "auto"
>> triggers colorization during regression testing. I have not found a
>
> That reason is obvious, dejagnu (expect?) creates pseudo terminals, so
> isatty is true, we'd need to just use -fno-diagnostics-color by default
> for the testsuite (IMHO not a big deal).
>
Fine for me.
> Anyway, I've kept the default as never for now, but am sending my review
> comments in form of a new diff, which fixes formatting, avoids memory leaks
> and changes it to introduce more color names (for caret, locus, quoted
> text), change default of note color (for some color compatibility with
> clang, bold green is there used for caret lines, for notes they use
> bold black apparently, but that doesn't work too well on white-on-black
> terminals). Right now the patch is unfinished, because there is no support
> for the new %[locus]%s:%d:%d%[] style diagnostics strings (where
> %[locus] and %[] stand for switching to "locus" color and resetting color
> %back) in the -Wformat code (and gettext). I'm wondering if instead of the
> %[colorname] and %[] it wouldn't be better to just have some %r or whatever
> letter isn't taken yet which would consume a const char * colorname from
> %va_arg, and some other letter with no argument that would do color reset.
> Ideas for best unused letters for that? Perhaps then -Wformat support for
> it would be easier. I.e. instead of:
> pp_printf ("%[locus]%s:%d:%d[]", loc.file, loc.line, loc.column);
> one would write:
> pp_printf ("%r%s:%d:%d%R", "locus", loc.file, loc.line, loc.column);
Thanks for working on this, your improvements are quite nice.
About %r versus %[colorname], I just don't see the user-case for
dynamic color names.
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.
Cheers,
Manuel.
