[Lldb-commits] [PATCH] D104422: [trace] Add a TraceCursor class

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 11 inline comments as done.
wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:39-40
+///
+///  The Trace initially points to a dummy invalid instruction error signaling
+///  the end of a trace, similar to a C++ collections' end iterator.
+///
----------------
clayborg wrote:
> Should the TraceCursor point to the last instruction more like a begin() STL call?
yes, i overcomplicated it


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:116-117
+  /// \return
+  ///     The size in bytes of the instruction opcode the cursor is pointing at.
+  ///     If the cursor points to an error in the trace, return \b 0.
+  virtual size_t GetInstructionSize() = 0;
----------------
clayborg wrote:
> Do we even need the instruction size in this cursor class interface? Clients can easily grab an instruction from GetLoadAddress() and do that themselves?
yes, this makes no sense. I sometimes use it for determining if two instructions are sequential, but i might as well use the control flow type to check that, i.e. if it's a normal or not taken branch, then the next instruction is sequential.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:72-75
+  /// \return
+  ///     \b true if the cursor is pointing to an error in the trace. The actual
+  ///     error message can be gotten by calling \a TraceCursor::GetError().
+  bool IsError();
----------------
clayborg wrote:
> Do we need this if we have GetError?
i imagine it might not always be a no-op to construct and error message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104422