[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 ↗

(On Diff #219344)

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

432 ↗

(On Diff #219344)

Delete enum

[jhenderson]

llvm/include/llvm/Support/raw_ostream.h

92 ↗

(On Diff #219344)

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

41–42

... then you can remove {}

llvm/include/llvm/Support/raw_ostream.h

90–93 ↗

(On Diff #219465)

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

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

309 ↗

(On Diff #219465)

and this {}

401 ↗

(On Diff #219465)

and this as well

[seiya]

Addressed review comments

[ruiu]

llvm/lib/Support/WithColor.cpp

73

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

73

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

 //===- WithColor.h ----------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_WITHCOLOR_H
 #define LLVM_SUPPORT_WITHCOLOR_H
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
+#include <stack>
 
 namespace llvm {
 
 extern cl::OptionCategory ColorCategory;
 
 class raw_ostream;
 
 // Symbolic names for various syntax elements.
enum class HighlightColor {
   Remark
 };
 
 /// An RAII object that temporarily switches an output stream to a specific
 /// color.
 class WithColor {
   raw_ostream &OS;
   bool DisableColors;
 
 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";
   /// @param OS The output stream
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 = "",
public:
   WithColor &changeColor(raw_ostream::Colors Color, bool Bold = false,
                          bool BG = false);
 
   /// Reset the colors to terminal defaults. Call this when you are done
   /// outputting colored text, or before program exit.
   WithColor &resetColor();
 };
 
+/// A color settings saved in NestableWithColorContext.
+struct ColorState {
+  raw_ostream::Colors Color;
+  bool Bold;
+  bool BG;
+
+  ColorState(raw_ostream::Colors Color, bool Bold, bool BG)
+      : Color(Color), Bold(Bold), BG(BG) {}
+};
+
+/// A context for NestableWithColor which holds saved colors.
+class NestableWithColorContext {
+  std::stack<ColorState> NestedColors;
+
+public:
+  void pushColor(raw_ostream::Colors Color, bool Bold, bool BG) {
+    NestedColors.emplace(Color, Bold, BG);
+  }
+
+  void popColor() { NestedColors.pop(); }
+
+  bool empty() { return NestedColors.empty(); }
+
+  const ColorState &currentColor() { return NestedColors.top(); }
+};
+
+/// A RAII object which temporarily changes the color in a specific stream just
+/// like WithColor, but supports restoring the previous color in the given
+/// context.
+///
+/// Usage:
+///
+///   NestableWithColorContext Ctx;
+///   NestableWithColor WNC1(outs(), Ctx, raw_ostream::RED, ...);
+///   {
+///     NestableWithColor WNC2(outs(), Ctx, raw_ostream::BLUE, ...);
+///     outs() << "A blue-colored text.";
+///     // WNC2 is destroyed and WNC1's color is restored here.
+///   }
+///   outs() << "A red-colored text.";
+///
+class NestableWithColor {
+  raw_ostream &OS;
+  NestableWithColorContext &Ctx;
+  bool DisableColors;
+
+public:
+  NestableWithColor(raw_ostream &OS, NestableWithColorContext &Ctx,
+                    raw_ostream::Colors Color = raw_ostream::SAVEDCOLOR,
+                    bool Bold = false, bool BG = false,
+                    bool DisableColors = false);
+  ~NestableWithColor();
+
+  template <typename T> NestableWithColor &operator<<(T &O) {
+    OS << O;
+    return *this;
+  }
+
+  template <typename T> NestableWithColor &operator<<(const T &O) {
+    OS << O;
+    return *this;
+  }
+};
+
 } // end namespace llvm
 
 #endif // LLVM_LIB_DEBUGINFO_WITHCOLOR_H

llvm/lib/Support/WithColor.cpp

 
 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: ";
 }
 
 raw_ostream &WithColor::warning(raw_ostream &OS, StringRef Prefix,
                                 bool DisableColors) {
   if (!Prefix.empty())
     OS << Prefix << ": ";
   return WithColor(OS, HighlightColor::Warning, DisableColors).get()
          << "warning: ";

 
 WithColor &WithColor::resetColor() {
   if (colorsEnabled())
     OS.resetColor();
   return *this;
 }
 
 WithColor::~WithColor() { resetColor(); }
+
+NestableWithColor::NestableWithColor(raw_ostream &OS,
+                                     NestableWithColorContext &Ctx,
+                                     raw_ostream::Colors Color, bool Bold,
+                                     bool BG, bool DisableColors)
+    : OS(OS), Ctx(Ctx), DisableColors(DisableColors) {
+  if (DisableColors)
+    return;
+
+  Ctx.pushColor(Color, Bold, BG);
+  if (Color == raw_ostream::RESET)
+    OS.resetColor();
+  else
+    OS.changeColor(Color, Bold, BG);
+}
+
+NestableWithColor::~NestableWithColor() {
+  if (DisableColors)
+    return;
+
+  Ctx.popColor();
+  // Restore the previous color if it exists.
+  if (!Ctx.empty()) {
+    const ColorState &State = Ctx.currentColor();
+    if (State.Color == raw_ostream::RESET)
+      OS.resetColor();
+    else
+      OS.changeColor(State.Color, State.Bold, State.BG);
+  }
+}