Page MenuHomePhabricator

[Support] Support restoring colors in WithColor
AcceptedPublic

Authored by seiya on Sep 2 2019, 12:23 AM.

Details

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.

Diff Detail

Event Timeline

seiya created this revision.Sep 2 2019, 12:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2019, 12:23 AM
seiya added a comment.Sep 2 2019, 12:25 AM

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

seiya edited the summary of this revision. (Show Details)Sep 2 2019, 12:41 AM
seiya edited the summary of this revision. (Show Details)

Added reviewers who may be familiar with this part.

ruiu added a comment.Sep 2 2019, 12:54 AM

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?

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 added a comment.EditedSep 2 2019, 9:46 PM

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 added a comment.Sep 3 2019, 12:54 AM

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 added a comment.Sep 3 2019, 1:18 AM

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 updated this revision to Diff 218573.Sep 3 2019, 6:27 PM
seiya retitled this revision from [Support] Add NestableWithColor to [Support] Support restoring colors in WithColor.
seiya edited the summary of this revision. (Show Details)

Moved NestableWithColor features into WithColor.

seiya added a comment.Sep 3 2019, 6:33 PM

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 marked an inline comment as done.Sep 3 2019, 6:51 PM
seiya added inline comments.
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 added a comment.Sep 3 2019, 10:50 PM

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 added a comment.Sep 3 2019, 11:57 PM

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 added a comment.Sep 4 2019, 12:08 AM

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 added a comment.Sep 4 2019, 2:37 AM

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 updated this revision to Diff 219344.Sep 9 2019, 7:22 AM
seiya edited the summary of this revision. (Show Details)
  • Abandon WithColorContext: save the previous color settings in WithColor instead.
  • Update the description.

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 added inline comments.Sep 9 2019, 8:33 AM
llvm/include/llvm/Support/raw_ostream.h
92

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

seiya updated this revision to Diff 219465.Sep 9 2019, 7:11 PM

Addressed review comments.

seiya marked 3 inline comments as done.Sep 9 2019, 7:11 PM
ruiu added a comment.Sep 9 2019, 8:22 PM

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 added inline comments.Sep 9 2019, 8:26 PM
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 updated this revision to Diff 219496.Sep 10 2019, 2:13 AM

Addressed review comments

ruiu added inline comments.Sep 10 2019, 2:22 AM
llvm/lib/Support/WithColor.cpp
73

Unrelated change?

seiya marked 4 inline comments as done.Sep 10 2019, 2:24 AM

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 updated this revision to Diff 219500.Sep 10 2019, 2:27 AM
  • Removed an unrelated change from the diff.
seiya marked 2 inline comments as done.Sep 10 2019, 2:27 AM
seiya added inline comments.
llvm/lib/Support/WithColor.cpp
73

Yes. Fixed.

ruiu accepted this revision.Sep 10 2019, 2:47 AM

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.

This revision is now accepted and ready to land.Sep 10 2019, 2:47 AM