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