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

[Chris Lattner]

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:

+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.

+      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.

+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.

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


+  // 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.

+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?  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.

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!

-Chris