[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

[Vedant Kumar via Phabricator via lldb-commits]

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