[Lldb-commits] [PATCH] D159164: [lldb] Add assembly syntax highlighting

[Greg Clayton via Phabricator via lldb-commits]

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