[PATCH] D81110: [Support] Move color handling from raw_fd_ostream to raw_ostream

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I think this is starting to look ok. There's still the constructor-calling-virtual issue that needs to be resolved though...



================
Comment at: llvm/include/llvm/Support/raw_ostream.h:107
+    // Use has_colors() to compute the default.
+    enable_colors(has_colors());
   }
----------------
This is equivalent to `enable_colors(false)` because it will not be calling the overriden methods of derived classes (as they don't exist yet) -- the "calling virtual functions from a constructor is weird" problem.

I think it makes sense that the default color state of a generic stream is "disabled".  That is basically the status quo, and if someone really wants to enable colors on a string_stream, I don't think it's unreasonable to ask him to explicitly enable that.

`raw_fd_ostream` can override this behavior. The status quo is that `raw_fd_ostream` hardcodes `ColorEnabled = true`, but I don't think it's unreasonable to change that to `ColorEnabled = has_colors()` like you did (or wanted to do).


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:522
   explicit raw_string_ostream(std::string &O) : OS(O) {
+    enable_colors(false);
     SetUnbuffered();
----------------
Per the above comment, I guess this is not really needed?

Also, when you're making changes to raw_string_ostream, don't forget about raw_svector_ostream -- they really shouldn't diverge on color handling.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:507-508
+
+  if (sys::Process::ColorNeedsFlush())
+    flush();
+  const char *colorcode =
----------------
jhenderson wrote:
> labath wrote:
> > This part worries me. The reason why this code is so convoluted and calls out to `sys::Process` is because on windows, one cannot always just write an ansi code in order to change the color (that functionality only arrived in windows ~10, and it is off by default). The way this code gets around that is by having the `sys::Process::` functions call some windows-specific APIs to achieve the color change.
> > 
> > This means that in the cases we don't have ansi emulation enabled, changing the "color" of a raw_string_ostream will result in twiddling of a color of the real terminal that the current process is attached to. I'm not entirely sure what are the effects of that (at the very least, it can cause text which is concurrently displayed to the console be randomly colored), but it definitely does not seem like an ideal state to be in.
> > 
> > This was sort of already a problem with raw_fd_ostream, where if you call `enable_colors` on a random stream, you'd get strange effects. However, if you only did that on streams where `has_colors` returned `true`, everything was ok.
> > 
> > But now, we're going to be routinely calling `enable_colors` on streams with `has_colors() == false`, so I think we should come up with a story of how can one be sure that writing a color to a string stream won't mess with global state.
> > 
> > One solution would be to say that one should call a function like `maybe_static bool raw_ostream::colors_use_ansi_codes()` (whose implementation could simply be `return !sys::Process::ColorNeedsFlush()`) before he tries to enable colors on these streams. That would be easy to implement but also easy to use incorrectly.
> > 
> > On the other end of the spectrum, we could refactor the `raw_ostream<->sys::Process<->ANSI` interface. For example, instead of having the Process class vend ansi codes, they could be embedded directly into the raw_ostream class. Most raw_ostream classes would use these directly. However, `raw_fd_ostream::changeColor` would do something like:
> > ```
> > if (is_displayed() && sys::Process::colorsNeedFancyHandling()) {
> >   flush();
> >   if (colors == SAVEDCOLOR)
> >     sys::Process::OutputBold(bg); //this now returns void
> >   else
> >     sys::Process::OutputColor(static_cast<char>(colors), bold, bg);
> >   return *this;
> > }
> > return raw_ostream::changeColor(colors, bold, bg);
> > ```
> > That would require some amount of refactoring (though hopefully not too much), but it would enable one to always get ansi escape codes out.
> > 
> > OTOH, it's not clear how useful it is to be able to get ansi escape codes if your system can't print them -- it would definitely be useful for testing but in reality they are most likely bound to end up on a terminal sooner or later). So it may be best to just disable printing ansi escape codes to *all* streams, just do it in a less shoot-footy way. One way to do that would be to make `enable_colors` a no-op on non-displayed streams if fancy handling is required. Maybe
> > ```
> > raw_ostream::enable_colors(bool Enable) {
> >   if (sys::Process::colorsNeedFancyHandling() && !is_displayed() /* or maybe has_colors()?*/)
> >     return;
> >   ColorEnabled = Enable;
> > }
> > ```
> > ?
> > 
> > Other options are possible...
> This may be completely unrelated, but something I wanted to throw out there - sometimes, at least on Windows, if you Ctrl-C in the middle of a process run, or sometimes when you get a crash, the console's colour is permanently changed to something. I have no idea why, but @labath's comments about the terminal's colour itself being changed made me wonder if this was somehow related.
Yes, that could be related, though the relationship is probably not so straightforward. You can definitely get the same effect also on unix with ansi escape codes. I'm guessing that the reason why you haven't seen this before is because unix shells typically have coloured prompts, which will override whatever state has the crashed process left the terminal in. You can observe this by killing an interactive application which disables terminal echo or other fancy tty flags -- shells don't typically reset these, and the result can be quite amusing (protip: typing "stty sane" blindly usually fixes things).

In fact, windows may be in a better position to do something about this, since it's the os itself that interprets the color changes (and not a random application sitting on the other end of the pty reading multiplexed output without knowing where it came from) and could reset the state when an application exits. Whether it does that -- I don't know. Based on your description, the answer is probably no.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:535-536
+
+  if (sys::Process::ColorNeedsFlush())
+    flush();
+  if (const char *colorcode = sys::Process::ResetColor())
----------------
(just thinking out loud) It might be nice to move this bit into the `use_color` function. Then maybe the function should be renamed to something that suggests it can have side effects... `prepare_colors` ?


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:367
+    Sos.changeColor(raw_ostream::YELLOW);
+    EXPECT_FALSE(Sos.str().empty());
+#ifdef LLVM_ON_UNIX
----------------
I'm guessing this check will fail on windows in the "non-ansi" mode, as changing colors there does not produce any "output" whatsoever. Right now, I don't think we're able to assert anything useful on windows. (well.. maybe we could say that the string is either empty or contains the appropriate ansi code).

@MaskRays suggestion for calling `sys::Process:UseAnsiEscapeCodes(true)` will make this always output ansi codes (regardless of whether the os supports them). The main problem with that is that the call changes global state which can effect the execution of other tests. We could save the original value of that flag and restore it, except that it is currently not possible to get the value of that flag (well... it is kinda exposed through `ColorNeedsFlush`, but that's a bit weird...)



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81110/new/

https://reviews.llvm.org/D81110