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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

two very minor comments, but looks good - I'm sure you already have, but be sure to run the dumper unittest test to ensure the output didn't change unexpectedly as a result some minor formatting mistake in this diff 🙂



================
Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:26
+  lldb::InstructionSP instruction;
+  lldb_private::ExecutionContext exe_ctx;
+};
----------------
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
```


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124064