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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
