[Support] Support restoring colors in WithColor

[seiya]

Summary

WithColor provides a way to change text color temporarily but currently it resets colors
in the destructor regardless of the previous color. Consider this example:

{
  WithColor Red(outs(), raw_ostream::RED);
  Red << "AAA\n";
  WithColor(outs(), raw_ostream::BLUE) << "BBB\n";
  Red << "CCC\n";
}

In this example "CCC" is not colored with red because the WithColor instance for
"BBB" resets the color in its destructor.

This patch addresses the issue by saving the previous color state and restoring
the color settings in the destructor.

[seiya]

This patch does not include tests but it will be used in D65191.

[seiya]

Added reviewers who may be familiar with this part.

[ruiu]

Instead of adding a new class, do you think you can change the behavior of WithColor so that it restores the original colors instead of resetting it?

[jhenderson]

In D67060#1654259, @ruiu wrote:

Instead of adding a new class, do you think you can change the behavior of WithColor so that it restores the original colors instead of resetting it?

This would be my preference too, if it's viable. I don't think WithColor has been around that long, so I suspect it may be straightforward to identify existing usage patterns, and I'd be surprised if any expect restoration to default.

[MaskRay]

Reusing WithColor is also my preference. I think you can change the third argument DisableColors to WithColorContext *Context = nullptr in the following overload:

static raw_ostream &error(raw_ostream &OS, StringRef Prefix = "",
                          bool DisableColors = false);

And make DisableColors a parameter with a default argument of the constructor of WithColorContext

[seiya]

In D67060#1654353, @jhenderson wrote:

In D67060#1654259, @ruiu wrote:

Instead of adding a new class, do you think you can change the behavior of WithColor so that it restores the original colors instead of resetting it?

This would be my preference too, if it's viable. I don't think WithColor has been around that long, so I suspect it may be straightforward to identify existing usage patterns, and I'd be surprised if any expect restoration to default.

I didn't came up with a good idea to add this feature to WithColor with the compatibility with existing usage but @MaskRay's idea looks good to me. I'll try the idea.

[ruiu]

Did you find any existing usage of WithColor that depends on the behavior of resetting the color? If not, you don't have to care too much about keeping the compatibility, as that is not really an intended feature as far as I understand. You probably can just change the behavior of WithColor so that it restores to the previous color instead of resseting.

[seiya]

Moved NestableWithColor features into WithColor.

[seiya]

In D67060#1655157, @ruiu wrote:

Did you find any existing usage of WithColor that depends on the behavior of resetting the color? If not, you don't have to care too much about keeping the compatibility, as that is not really an intended feature as far as I understand. You probably can just change the behavior of WithColor so that it restores to the previous color instead of resseting.

I grep'ed the existing usage and ran tests in llvm/test/tools but it looks that no tests relies on the behavior. As you say it's safe to change the behavior I think.

[seiya]

llvm/include/llvm/Support/WithColor.h

87

In D67060#1655078, @MaskRay suggested to replace bool DisableColors with WithColorContext *Context. IIUC, the usage will look like:

WithColorContext Ctx(/*DisableColors=*/ false);
{
  WithColor Red(outs(), raw_ostream::RED, false, false, &Ctx);
  Red << "AAA\n";
  WithColor(outs(), raw_ostream::BLUE, false, false, &Ctx) << "BBB\n";
  Red << "CCC\n";
}

However, with this approach we need to Bold and BG every time we use WithColorContext. How about adding a new constructor (b):

// (a) Use DisableColors.
WithColor(raw_ostream &OS,
          raw_ostream::Colors Color = raw_ostream::SAVEDCOLOR,
          bool Bold = false, bool BG = false, bool DisableColors = false,
          WithColorContext *Context = nullptr);

// (b) Use Context->DisableColors.
WithColor(raw_ostream &OS, WithColorContext *Context,
          raw_ostream::Colors Color = raw_ostream::SAVEDCOLOR,
          bool Bold = false, bool BG = false)

[ruiu]

This looks better, but I don't think we need the concept of context. If a function takes a context object, it is usually expected that you can interlace function calls with different context objects freely. Say, you have context object A and B, then you literally have two contexts, and they don't interfere with each other. However, that's not the case for the terminal color. The current color of text output on console is naturally and fundamentally a global state associated to each terminal, and therefore there is only one context. So, I don't think that allowing multiple contexts captures the real concept correctly.

I believe the following scheme models captures the reality better:

1. Add a new method, getColor, to raw_fd_stream, which returns the current color set by changeColor before
2. Make a change to WithColor so that it remembers the current color (obtained by calling getColor) before changing color
3. In the destructor, WithColor restores the previous color

[seiya]

In D67060#1656821, @ruiu wrote:

This looks better, but I don't think we need the concept of context. If a function takes a context object, it is usually expected that you can interlace function calls with different context objects freely. Say, you have context object A and B, then you literally have two contexts, and they don't interfere with each other. However, that's not the case for the terminal color. The current color of text output on console is naturally and fundamentally a global state associated to each terminal, and therefore there is only one context. So, I don't think that allowing multiple contexts captures the real concept correctly.

I believe the following scheme models captures the reality better:

1. Add a new method, getColor, to raw_fd_stream, which returns the current color set by changeColor before
2. Make a change to WithColor so that it remembers the current color (obtained by calling getColor) before changing color
3. In the destructor, WithColor restores the previous color

That sounds good idea to me too but how can we obtain the current color from the terminal? I skimmed xterm control sequences but couldn't find out the feature (note: I'm not familiar with Windows).

Also, getting colors from the terminal may degrades the performance since we need to invoke a system call every time we use WithColor. We should discuss about the performance after measuring it, btw.

[ruiu]

I don't think there's a way to get the current color from the terminal, so you should modify changeColor so that the function remembers the color given to the function.

[seiya]

In D67060#1656873, @ruiu wrote:

I don't think there's a way to get the current color from the terminal, so you should modify changeColor so that the function remembers the color given to the function.

Oh I get it it's good idea! Will try it.

[seiya]

• Abandon WithColorContext: save the previous color settings in WithColor instead.
• Update the description.

[MaskRay]

The new approach looks good.

llvm/include/llvm/Support/raw_ostream.h

94

The struct will be an aggregate if you delete the constructor. It can still be brace initialized.

430

Delete enum

[jhenderson]

llvm/include/llvm/Support/raw_ostream.h

92

BG is not an obvious abbreviation for me. Could you use an unabbreviated name please?

[seiya]

Addressed review comments.

[ruiu]

This patch looks good, but I'd add a test for the new code. Is there any place in LLVM where the current color is saved and restored? If there's such code, you can rewrite the code to see if your code would work as expected.

[ruiu]

llvm/include/llvm/Support/WithColor.h

40–41

... then you can remove {}

llvm/include/llvm/Support/raw_ostream.h

90–93

C++11 allows to write default values for member variables, i.e.

Colors Color = Colors::RESET;
bool Bold = false;
bool Background = false;

309

and this {}

401

and this as well

[seiya]

Addressed review comments

[ruiu]

llvm/lib/Support/WithColor.cpp

72

Unrelated change?

[seiya]

In D67060#1664226, @ruiu wrote:

This patch looks good, but I'd add a test for the new code. Is there any place in LLVM where the current color is saved and restored? If there's such code, you can rewrite the code to see if your code would work as expected.

There're no such code AFAIK. How about committing this patch and D65191 at once? It will include a test case which depends on this feature.

[seiya]

• Removed an unrelated change from the diff.

[seiya]

llvm/lib/Support/WithColor.cpp

72

Yes. Fixed.

[ruiu]

LGTM

If you are going to submit another change to test this code, it is fine to submit this patch separately. Thank you for doign this.

llvm/include/llvm/Support/WithColor.h

enum class HighlightColor {
   Remark
 };
 
 /// An RAII object that temporarily switches an output stream to a specific
 /// color.
 class WithColor {
   raw_ostream &OS;
   bool DisableColors;
+  raw_ostream::ColorState PrevColor;
 
 public:
   /// To be used like this: WithColor(OS, HighlightColor::String) << "text";
   /// @param OS The output stream
   /// @param S Symbolic name for syntax element to color
   /// @param DisableColors Whether to ignore color changes regardless of -color
   /// and support in OS
   WithColor(raw_ostream &OS, HighlightColor S, bool DisableColors = false);
   /// To be used like this: WithColor(OS, raw_ostream::Black) << "text";
public:
   static raw_ostream &warning();
   /// Convenience method for printing "note: " to stderr.
   static raw_ostream &note();
   /// Convenience method for printing "remark: " to stderr.
   static raw_ostream &remark();
 
   /// Convenience method for printing "error: " to the given stream.
   static raw_ostream &error(raw_ostream &OS, StringRef Prefix = "",
                             bool DisableColors = false);
   /// Convenience method for printing "warning: " to the given stream.
   static raw_ostream &warning(raw_ostream &OS, StringRef Prefix = "",
                               bool DisableColors = false);
   /// Convenience method for printing "note: " to the given stream.
   static raw_ostream &note(raw_ostream &OS, StringRef Prefix = "",
                            bool DisableColors = false);
   /// Convenience method for printing "remark: " to the given stream.
   static raw_ostream &remark(raw_ostream &OS, StringRef Prefix = "",

llvm/include/llvm/Support/raw_ostream.h

enum class Colors {
     BLUE,
     MAGENTA,
     CYAN,
     WHITE,
     SAVEDCOLOR,
     RESET,
   };
 
+  /// Color settings.
+  struct ColorState {
+    Colors Color = Colors::RESET;
+    bool Bold = false;
+    /// If true, the background color is changed instead of the foreground.
+    bool Background = false;
+  };
+
   static const Colors BLACK = Colors::BLACK;
   static const Colors RED = Colors::RED;
   static const Colors GREEN = Colors::GREEN;
   static const Colors YELLOW = Colors::YELLOW;
   static const Colors BLUE = Colors::BLUE;
   static const Colors MAGENTA = Colors::MAGENTA;
   static const Colors CYAN = Colors::CYAN;
   static const Colors WHITE = Colors::WHITE;
public:
 
   /// This function determines if this stream is displayed and supports colors.
   virtual bool has_colors() const { return is_displayed(); }
 
   // Enable or disable colors. Once disable_colors() is called,
   // changeColor() has no effect until enable_colors() is called.
   virtual void enable_colors(bool /*enable*/) {}
 
+  /// Returns the current color set through the raw_ostream interface
+  /// (changeColor, etc).
+  virtual ColorState getColor() { return {}; }
+
   //===--------------------------------------------------------------------===//
   // Subclass Interface
   //===--------------------------------------------------------------------===//
 
 private:
   /// The is the piece of the class that is implemented by subclasses.  This
   /// writes the \p Size bytes starting at
   /// \p Ptr to the underlying stream.

 
 /// A raw_ostream that writes to a file descriptor.
 ///
 class raw_fd_ostream : public raw_pwrite_stream {
   int FD;
   bool ShouldClose;
   bool SupportsSeeking;
   bool ColorEnabled = true;
+  ColorState CurrentColor;
 
 #ifdef _WIN32
   /// True if this fd refers to a Windows console device. Mintty and other
   /// terminal emulators are TTYs, but they are not consoles.
   bool IsWindowsConsole = false;
 #endif
 
   std::error_code EC;
#endif
   /// Determine an efficient buffer size.
   size_t preferred_buffer_size() const override;
 
   /// Set the flag indicating that an output error has been encountered.
   void error_detected(std::error_code EC) { this->EC = EC; }
 
   void anchor() override;
 
+  void saveColor(Colors Color, bool Bold, bool BG) {
+    CurrentColor = {Color, Bold, BG};
+  }
+
 public:
   /// Open the specified file for writing. If an error occurs, information
   /// about the error is put into EC, and the stream should be immediately
   /// destroyed;
   /// \p Flags allows optional flags to control how the file will be opened.
   ///
   /// As a special case, if Filename is "-", then the stream will use
   /// STDOUT_FILENO instead of opening a file. This will not close the stdout
public:
   uint64_t seek(uint64_t off);
 
   raw_ostream &changeColor(enum Colors colors, bool bold=false,
                            bool bg=false) override;
   raw_ostream &resetColor() override;
 
   raw_ostream &reverseColor() override;
 
+  ColorState getColor() override { return CurrentColor; }
+
   bool is_displayed() const override;
 
   bool has_colors() const override;
 
   void enable_colors(bool enable) override { ColorEnabled = enable; }
 
   std::error_code error() const { return EC; }
 

llvm/lib/Support/WithColor.cpp

UseColor("color", cl::cat(ColorCategory),
              cl::init(cl::BOU_UNSET));
 
 WithColor::WithColor(raw_ostream &OS, HighlightColor Color, bool DisableColors)
     : OS(OS), DisableColors(DisableColors) {
   // Detect color from terminal type unless the user passed the --color option.
   if (colorsEnabled()) {
     switch (Color) {
     case HighlightColor::Address:
-      OS.changeColor(raw_ostream::YELLOW);
+      changeColor(raw_ostream::YELLOW);
       break;
     case HighlightColor::String:
-      OS.changeColor(raw_ostream::GREEN);
+      changeColor(raw_ostream::GREEN);
       break;
     case HighlightColor::Tag:
-      OS.changeColor(raw_ostream::BLUE);
+      changeColor(raw_ostream::BLUE);
       break;
     case HighlightColor::Attribute:
-      OS.changeColor(raw_ostream::CYAN);
+      changeColor(raw_ostream::CYAN);
       break;
     case HighlightColor::Enumerator:
-      OS.changeColor(raw_ostream::MAGENTA);
+      changeColor(raw_ostream::MAGENTA);
       break;
     case HighlightColor::Macro:
-      OS.changeColor(raw_ostream::RED);
+      changeColor(raw_ostream::RED);
       break;
     case HighlightColor::Error:
-      OS.changeColor(raw_ostream::RED, true);
+      changeColor(raw_ostream::RED, true);
       break;
     case HighlightColor::Warning:
-      OS.changeColor(raw_ostream::MAGENTA, true);
+      changeColor(raw_ostream::MAGENTA, true);
       break;
     case HighlightColor::Note:
-      OS.changeColor(raw_ostream::BLACK, true);
+      changeColor(raw_ostream::BLACK, true);
       break;
     case HighlightColor::Remark:
-      OS.changeColor(raw_ostream::BLUE, true);
+      changeColor(raw_ostream::BLUE, true);
       break;
     }
   }
 }
 
 raw_ostream &WithColor::error() { return error(errs()); }
 
 raw_ostream &WithColor::warning() { return warning(errs()); }
 
 raw_ostream &WithColor::note() { return note(errs()); }
 
 raw_ostream &WithColor::remark() { return remark(errs()); }
 
 raw_ostream &WithColor::error(raw_ostream &OS, StringRef Prefix,
                               bool DisableColors) {
   if (!Prefix.empty())
     OS << Prefix << ": ";
-  return WithColor(OS, HighlightColor::Error, DisableColors).get()
-         << "error: ";
+  return WithColor(OS, HighlightColor::Error, DisableColors).get() << "error: ";
 }
 
 raw_ostream &WithColor::warning(raw_ostream &OS, StringRef Prefix,
                                 bool DisableColors) {
   if (!Prefix.empty())
     OS << Prefix << ": ";
   return WithColor(OS, HighlightColor::Warning, DisableColors).get()
          << "warning: ";
if (DisableColors)
     return false;
   if (UseColor == cl::BOU_UNSET)
     return OS.has_colors();
   return UseColor == cl::BOU_TRUE;
 }
 
 WithColor &WithColor::changeColor(raw_ostream::Colors Color, bool Bold,
                                   bool BG) {
-  if (colorsEnabled())
+  if (colorsEnabled()) {
+    PrevColor = OS.getColor();
     OS.changeColor(Color, Bold, BG);
+  }
+
   return *this;
 }
 
 WithColor &WithColor::resetColor() {
   if (colorsEnabled())
     OS.resetColor();
   return *this;
 }
 
-WithColor::~WithColor() { resetColor(); }
+WithColor::~WithColor() {
+  if (colorsEnabled()) {
+    if (PrevColor.Color == raw_ostream::Colors::RESET)
+      OS.resetColor();
+    else
+      OS.changeColor(PrevColor.Color, PrevColor.Bold, PrevColor.Background);
+  }
+}

llvm/lib/Support/raw_ostream.cpp

const char *colorcode =
           ? sys::Process::OutputBold(bg)
           : sys::Process::OutputColor(static_cast<char>(colors), bold, bg);
   if (colorcode) {
     size_t len = strlen(colorcode);
     write(colorcode, len);
     // don't account colors towards output characters
     pos -= len;
   }
+
+  saveColor(colors, bold, bg);
   return *this;
 }
 
 raw_ostream &raw_fd_ostream::resetColor() {
   if (!ColorEnabled)
     return *this;
 
   if (sys::Process::ColorNeedsFlush())
     flush();
   const char *colorcode = sys::Process::ResetColor();
   if (colorcode) {
     size_t len = strlen(colorcode);
     write(colorcode, len);
     // don't account colors towards output characters
     pos -= len;
   }
+
+  saveColor(Colors::RESET, false, false);
   return *this;
 }
 
 raw_ostream &raw_fd_ostream::reverseColor() {
   if (!ColorEnabled)
     return *this;
 
   if (sys::Process::ColorNeedsFlush())