[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:41
   m_pt_insn.iclass = ptic_error;
+  m_is_error = true;
 }
----------------
Is this boolean necessary? In the case of an error, the other two fields also indicate an error so this doesn't seem to add much information.
If we remove it, you can just update IsError to check the other two fields accordingly.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:40
 
-IntelPTInstruction::IntelPTInstruction(llvm::Error err) {
-  llvm::handleAllErrors(std::move(err),
-                        [&](std::unique_ptr<llvm::ErrorInfoBase> info) {
-                          m_error = std::move(info);
-                        });
+IntelPTInstruction::IntelPTInstruction(Error &err) {
   m_pt_insn.ip = LLDB_INVALID_ADDRESS;
----------------
Feels kinda weird that the `err` param isn't being used? Not sure if it would be preferred but another option would be to make the default constructor construct an error instruction.
Passing the error is more clear that this constructor will create an error instruction, but then it feels a bit unnecessary to pass it since it's now unused. I'm a little torn so would be curious to hear others opinions (:


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:156
+  template <typename... Ts> void AppendInstruction(Ts... __args) {
+    m_instructions.emplace_back(std::move(IntelPTInstruction(__args...)));
+  }
----------------
Should we be using `std::forward()` here? Same question for the `AppendError` function


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:77
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
 }
----------------
nit: should we update this to use the error map? I don't think there's a significant difference performance wise, but the code would be a little cleaner imo and consistent with how `GetError()` works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293