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

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

there are many comments from the previous versions of this diff that you didn't apply. Go through all of them first :)



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:88-91
+  // /// \return
+  // ///     An \a llvm::Error object if this class corresponds to an Error, or an
+  // ///     \a llvm::Error::success otherwise.
+  // llvm::Error ToError() const;
----------------
delete it. We don't want to leave old code as comments


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:122
   llvm::Optional<uint64_t> m_timestamp;
-  std::unique_ptr<llvm::ErrorInfoBase> m_error;
+  bool is_error;
 };
----------------
now that you changed this, could show share in the description of this diff the difference in byte size between the old and new code when tracing the same number of instructions?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:152
+  ///   The error of the trace.
+  llvm::Error GetError(uint64_t ins_idx);
+
----------------
or GetErrorByInstructionIndex


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:148-151
+  /// Get the error at some instruction index from the decoded trace.
+  ///
+  /// \return
+  ///   The error of the trace.
----------------
wallace wrote:
> we can make the documentation clearer
you didn't apply these changes


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:157
+
+  /// Append an error of instruction decoding.
+  void AppendError(llvm::Error err);
----------------
wallace wrote:
> 
same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293