[Lldb-commits] [PATCH] D124064: [NFC][trace] simplify the instruction dumper

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:26
+  lldb::InstructionSP instruction;
+  lldb_private::ExecutionContext exe_ctx;
+};
----------------
jj10306 wrote:
> It shouldn't be an issue now because this struct is never stored any where, but in the future we should be aware that if this struct is ever stored "long term" (ie in another class), we should instead use `ExececutionContextRef`
> From the documentation in `ExecutionContext.h`
> ```
> /// exist during a function that requires the objects. ExecutionContext
> /// objects should NOT be used for long term storage since they will keep
> /// objects alive with extra shared pointer references to these  objects
> ```
Yes, you are right :) 


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:199
+/// instruction's disassembler when possible.
+std::tuple<DisassemblerSP, InstructionSP>
+CalculateDisass(const InstructionSymbolInfo &insn_info,
----------------
jj10306 wrote:
> should this be a static function?
goot catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124064