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