[cfe-commits] [PATCH] Addition of color to -ast-dump

[Philip Craig]

On 24 January 2013 03:24, Matthieu Monrocq <matthieu.monrocq at gmail.com> wrote:
>
>
> On Wed, Jan 23, 2013 at 12:12 PM, Philip Craig <philipjcraig at gmail.com>
> wrote:
>>
>>
>>
>> ================
>> Comment at: include/clang/AST/DeclBase.h:853
>> @@ -852,2 +852,3 @@
>>    // Debuggers don't usually respect default arguments.
>>    LLVM_ATTRIBUTE_USED void dump() const;
>> +  // Same as dump(), but forces color printing.
>> ----------------
>> Saleem Abdulrasool wrote:
>> > Why not make colour a default valued parameter for dump?  This would
>> > avoid duplication of the method as well as reduce the interface.
>> As the comment states, debuggers don't handle default arguments.
>>
>> ================
>> Comment at: lib/AST/ASTDumper.cpp:113
>> @@ -58,1 +112,3 @@
>> +        LastLocFilename(""), LastLocLine(~0U),
>> +        ShowColors(SM && SM->getDiagnostics().getShowColors()) { }
>>
>> ----------------
>> I used OS.has_colors() in lib/Frontend/ASTConsumers.cpp. We probably
>> should be consistent between these. I don't know which is better.
>>
>>
>> http://llvm-reviews.chandlerc.com/D291
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> Is OS.has_colors() correctly set in case of pipes ? If not then it makes
> sense to use a user available flag.

has_colors() is basically a isatty() call. Using a user available flag
is better, but does it make sense to use the diagnostics flag?
Strictly speaking this isn't part of diagnostics. I don't feel
strongly about it though.

Also, if we do use the diagnostics flag, we could still fallback to
has_colors() when there is no SourceManager. Would this remove the
need for Stmt::dumpColors()?  As it is, I don't see the point of
Decl::dumpColors(), because we'll always have a SourceManager for
Decl, and color dumping will default to on anyway.