vsk added inline comments.
================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:26 +public: + IntelPTError() = default; + ---------------- Do we need a default constructor for this pure-virtual class? ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:116 + pt_insn m_pt_insn; + std::shared_ptr<IntelPTError> m_error; +}; ---------------- Still doesn't feel like this is aligning with the design of llvm::Error - they're supposed to be unique and lightweight. Specifically: - Why is the error instance shareable? Do multiple threads need to handle the same error? - For that matter, why is IntelPTInstruction copyable? - How much are we gaining by storing the libipt_error_code vs. storing a `bool m_is_gap` field? Imo it seems a bit too complicated to have three different ways to construct an IntelPTInstruction with an error. Would be nice to just have one? ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:36 + + IntelPTInstruction(const pt_insn &pt_insn, int libipt_error_code = 0) + : m_pt_insn(pt_insn), m_libipt_error_code(libipt_error_code), ---------------- vsk wrote: > Does this IntelPTInstruction constructor ever get called with a non-zero > libipt error code? Still wondering about this: why do we need to construct an IntelPTInstruction using a pt_insn as well as an error code? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
