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

[Pavel Labath via Phabricator via llvm-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

I think this (seemingly simple) patch is complex enough to be split into llvm&lldb parts.

For the llvm part, I think we also need to somehow address the fact that color handling on windows is.... unusual. More on that in my inline comment.

PS: The last upload appears to be broken -- a partial diff only.



================
Comment at: lldb/include/lldb/Utility/Stream.h:408
+        : llvm::raw_ostream(/*unbuffered*/ true), m_target(target) {
+      enable_colors(true);
+    }
----------------
I don't think that enabling colors for all lldb streams is a good default. I think a better solution would be to make this a constructor argument and set it to true when constructing the Streams that are a part of the CommandReturnObject (if colors are enabled). Then we wouldn't need the m_colors variable. Would that work?


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


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

https://reviews.llvm.org/D81110