clayborg added a comment.
We shouldn't need to pass in "bool use_color" to the Disassembler creation
functions as the only reason to pass something to the creation function for any
plug-in is if that data would exclude a disassembler if it didn't support
color. But we always want to see some disassembly rather than none if a plug-in
is the only one that handles a certain architecture and doesn't support color.
We should either add accessors to enable colorization to the Disassembler
virtual plug-in interface _or_ add "bool use_color" to the needed functions.
Looks DisassembleBytes added a "bool use_color" argument to the call already,
so we shouldn't need a "bool use_color" for the plug-in selection. Are there
other places that need to know about the "use_color" setting that do not
results from a direct function call that can have an argument added to it?
Even if colorization is enabled, It would still expect any APIs that return
information in individual instructions, like:
const char *SBInstruction::GetMnemonic(lldb::SBTarget target);
const char *SBInstruction::GetOperands(lldb::SBTarget target);
const char *SBInstruction::GetComment(lldb::SBTarget target);
To not have colorization attached to the returned string.
================
Comment at: lldb/include/lldb/Core/Disassembler.h:409
+ static lldb::DisassemblerSP FindPlugin(const ArchSpec &arch,
+ const char *flavor, bool use_color,
+ const char *plugin_name);
----------------
We shouldn't need to pass "use_color" in when finding the disassembler plug-in.
We should add an accessor so the Disassembler virtual function list like:
```
bool Disassembler::SetUseColor(bool enable);
```
And the plug-in can return true if it supports colorization and false if not.
The selection of the disassembler plug-in shouldn't be predicated on color
support right?
================
Comment at: lldb/include/lldb/Core/Disassembler.h:416
const char *flavor,
+ bool use_color,
const char *plugin_name);
----------------
remove and use accessor suggested above?
================
Comment at: lldb/include/lldb/lldb-private-interfaces.h:33
+ const char *flavor,
+ bool use_color);
typedef DynamicLoader *(*DynamicLoaderCreateInstance)(Process *process,
----------------
remove per above comments
================
Comment at: lldb/source/Commands/CommandObjectDisassemble.cpp:456-457
- DisassemblerSP disassembler =
- Disassembler::FindPlugin(m_options.arch, flavor_string, plugin_name);
+ DisassemblerSP disassembler = Disassembler::FindPlugin(
+ m_options.arch, flavor_string, GetDebugger().GetUseColor(), plugin_name);
----------------
revert
================
Comment at: lldb/source/Core/Disassembler.cpp:59
DisassemblerSP Disassembler::FindPlugin(const ArchSpec &arch,
- const char *flavor,
+ const char *flavor, bool use_color,
const char *plugin_name) {
----------------
revert
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D159164/new/
https://reviews.llvm.org/D159164
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
