[Lldb-commits] [PATCH] D81058: [lldb/Interpreter] Color "error:" and "warning:" in the CommandReturnObject output.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this looks good now. All that's now left is a test case. I think it should be possible to test this in a number of ways:

- CommandReturnObject unittest
- a shell test which forces `use-color` to true
- a python test which forces `use-color` to true
- a pexpect test



================
Comment at: lldb/include/lldb/Utility/Stream.h:402-403
 
+    bool has_colors() const override { return true; }
+
     uint64_t current_pos() const override {
----------------
This is not consistent with the intend of this method (`This function determines if this stream is displayed and supports colors.`). One could argue whether this could be "true" for CommandObjectResult streams which are destined to be later printed to a terminal, but it's definitely not a good default.

However, all of that is irrelevant, as this should not be needed anymore. Is that correct?


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:212
 
-  CommandReturnObject result;
+  CommandReturnObject result(m_debugger.GetUseColor());
 
----------------
I'm wondering in how many places do we construct `CommandReturnObject`s. If it's not too many, it may make sense to remove the `=false` from the colors argument, to force the user to think about the right value...


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

https://reviews.llvm.org/D81058