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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: lldb/source/Interpreter/CommandReturnObject.cpp:49-51
+    llvm::WithColor(GetErrorStream().AsRawOstream(),
+                    llvm::HighlightColor::Error, m_colors)
+        << "error: ";
----------------
I assume you can't just use `llvm::WithColor::error()` here?

Same below.


================
Comment at: lldb/source/Interpreter/CommandReturnObject.cpp:79
+                  llvm::HighlightColor::Warning, m_colors)
+      << "warning: " << sstrm.GetString();
 }
----------------
Same as above, but with `WithColor::warning()`


================
Comment at: llvm/lib/Support/raw_ostream.cpp:507-508
+
+  if (sys::Process::ColorNeedsFlush())
+    flush();
+  const char *colorcode =
----------------
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.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:515-516
+
+raw_ostream &raw_ostream::changeColor(enum Colors colors, bool bold,
+                                         bool bg) {
+  if (!use_color())
----------------
Is this properly clang-formatted? It looks slightly weird to me.


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

https://reviews.llvm.org/D81110