[llvm-commits] [PATCH] ANSI colors for clang!

[Török Edwin]

On 2009-05-31 22:13, Chris Lattner wrote:
> On May 30, 2009, at 4:50 AM, Török Edwin wrote:
>   
>> The attached patches make clang output diagnostics in color!
>>     
>
> Very cool.  Here is some feedback on the LLVM part:
>   

Thanks for the feedback, new version attached.

> +static bool terminalHasColors()
> +{
> +  if (const char *term = std::getenv("TERM")) {
> ...
> +bool Process::StandardOutHasColors() {
> +  if (!StandardOutIsDisplayed())
>
>
> You use inconsistent brace style in your patch, please consistently  
> put the brace on the same line as the function name.
>   

Fixed.

> +      static const char *OutputColor(char c, bool bold, bool bg,  
> unsigned &Len);
> +      static const char *OutputBold(bool bg, unsigned &Len);
> +      static const char *ResetColor(unsigned savedColors, unsigned  
> &Len);
>
> Is there any reason here to return the length of the string?   
> Returning null terminated strings and using strlen should be fine.
>   

Premature optimization, but you're right strlen should be ok.

> +raw_ostream &raw_ostream::changeColor(enum Colors colors, bool bold,
> +                                                 bool bg)
>
> Funky indentation.
>
> +raw_ostream &raw_ostream::changeColor(enum Colors colors, bool bold,
> +                                                 bool bg)
> +{
> +  if (!Unbuffered && sys::Process::ColorNeedsFlush())
> +    flush();
> +  return *this;
>
> I don't think you need to check for unbuffered here (or in reset  
> color).  flush() on an unbuffered stream should be harmless.
>   

Ok.

> However, why implement this in raw_ostream at all?  Can't the base  
> versions of these methods be a noop?
>   

Because Unbuffered is not available in subclasses. Now that I no longer
need Unbuffered I made the body of these two to be 'return *this'.

>
> +  // Changes the foreground color of text, until the next changeColor,
> +  // or resetColor is encountered.
> +  virtual raw_ostream &changeColor(enum Colors colors, bool bold=false,
> +                                   bool  bg=false);
> +  virtual raw_ostream &resetColor();
>
> Please improve the doxygen comment for these to describe what the  
> arguments are, and what resetColor does.
>   

Done.

> +raw_ostream &raw_fd_ostream::changeColor(enum Colors colors, bool bold,
> +                                                 bool bg)
> +{
> +  if (!oldColors)
> +    oldColors = sys::Process::SaveCurrentColor();
>
> Does raw_fd_ostream really need a oldColors member? 

For win32 yes. On Unix its an unused field.

>  Is this to push/ 
> pop the current color?  If so, could the state be saved outside the  
> stream itself?  Maybe an RAII object that installs a new color and  
> saves the old one... restoring it when destroyed would be better?   
> That way raw_ostream could just have "give me your current color" and  
> "change color" interfaces, which would be very simple and nice.
>   

Since the field is only needed on win32 I'm afraid people will forget to
call getCurrentColor(),
and just pass in 0 or some other random data to resetColor() (because
that'd work on unix).

The problem comes from the fact that on Unix I can't find out what the
current color is,
but I don't need to since I can always reset to terminal defaults
(without knowing what that is).
However on win32 I have the opposite: I can find out what color I have,
and can reset to that, but I can't find out
what the default colors were. Hence I have to save the current color to
be able to reset on program termination.

Actually I don't know how big a problem this is on win32, terminals are
always black there AFAIK, so I could just reset
to that without saving the old color. But then I'd have to use ANSI
colors on mingw/cygwin since you can change the bg there ...

I'd prefer not saving the color at all if the oldColors field is a
problem, does it really increase memory consumption? :)

> A lot of the complexity of the patch looks like it has to do with  
> Windows support.  Please find someone with a real VC++ build to verify  
> that it actually works :)
>
> Thanks a lot for tackling this Edwin, it looks very useful!
>   

Thanks,
--Edwin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm.patch
Type: text/x-diff
Size: 9683 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090531/7bf8616f/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang.patch
Type: text/x-diff
Size: 7029 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090531/7bf8616f/attachment-0001.patch>