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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Core/Disassembler.h:158
                     bool show_bytes, bool show_control_flow_kind,
-                    const ExecutionContext *exe_ctx,
+                    bool show_color, const ExecutionContext *exe_ctx,
                     const SymbolContext *sym_ctx,
----------------
We might not need "bool show_color" here if we can rely on the "exe_ctx->GetTarget()->GetDebugger()"?


================
Comment at: lldb/include/lldb/Core/Disassembler.h:343-344
   void Dump(Stream *s, bool show_address, bool show_bytes,
-            bool show_control_flow_kind, const ExecutionContext *exe_ctx);
+            bool show_control_flow_kind, bool show_color,
+            const ExecutionContext *exe_ctx);
 
----------------
Ditto here. Should we rely on the exe_ctx having a target and thus we can get to the debugger instead of passing "show_color" as a separate arg?


================
Comment at: lldb/include/lldb/Core/Disassembler.h:457-458
                          bool mixed_source_and_assembly,
-                         uint32_t num_mixed_context_lines, uint32_t options,
-                         Stream &strm);
+                         uint32_t num_mixed_context_lines, bool show_color,
+                         uint32_t options, Stream &strm);
 
----------------
should "show_color" be put as a bit into the "options" instead?


================
Comment at: lldb/source/API/SBInstruction.cpp:263-264
     FormatEntity::Parse("${addr}: ", format);
     inst_sp->Dump(&s.ref(), 0, true, false, /*show_control_flow_kind=*/false,
-                  nullptr, &sc, nullptr, &format, 0);
+                  /*show_color=*/false, nullptr, &sc, nullptr, &format, 0);
     return true;
----------------
If we rely on the execution context being NULL, then we don't need to pass in color as false here.


================
Comment at: lldb/source/API/SBInstruction.cpp:299
     inst_sp->Dump(&out_stream, 0, true, false, /*show_control_flow_kind=*/false,
-                  nullptr, &sc, nullptr, &format, 0);
+                  /*show_color=*/false, nullptr, &sc, nullptr, &format, 0);
   }
----------------
ditto


================
Comment at: lldb/source/API/SBInstructionList.cpp:169-170
         inst->Dump(&sref, max_opcode_byte_size, true, false,
-                   /*show_control_flow_kind=*/false, nullptr, &sc, &prev_sc,
-                   &format, 0);
+                   /*show_control_flow_kind=*/false, /*show_color=*/false,
+                   nullptr, &sc, &prev_sc, &format, 0);
         sref.EOL();
----------------
ditto


================
Comment at: lldb/source/Core/Disassembler.cpp:604-605
                        bool show_address, bool show_bytes,
-                       bool show_control_flow_kind,
+                       bool show_control_flow_kind, bool show_color,
                        const ExecutionContext *exe_ctx,
                        const SymbolContext *sym_ctx,
----------------
Should we just rely on the "exe_ctx" having a target here instead of adding a new parameter here? Cause we can do "exe_ctx->GetTarget()->GetDebugger()" if needed. If no exe_ctx, then assume no color?


================
Comment at: lldb/source/Core/Disassembler.cpp:658
+  if (opcode_name.length() >= opcode_column_width) {
+    opcode_column_width = opcode_name.length() + 1;
   }
----------------
The color codes bytes should't contribute to the opcode columns width. We should probably use "m_opcode_name", but we will need to guarantee that both are filled in if we ask for color. Can we modify it so if we ask for the opcode name with color that we always fill in the m_opcode_name in as well?


================
Comment at: lldb/source/Core/DumpDataExtractor.cpp:160
+            s, show_address, show_bytes, show_control_flow_kind,
+            target_sp->GetDebugger().GetUseColor(), &exe_ctx);
       }
----------------
Here is an example of where we fill the execution context in with the target, but also pass in "target_sp->GetDebugger().GetUseColor()"


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

https://reviews.llvm.org/D159164