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