[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 marked 13 inline comments as done.
jj10306 added inline comments.


================
Comment at: lldb/include/lldb/API/SBTraceCursor.h:166
+  ///     The specific kind of event the cursor is pointing at, or \b
+  ///     TraceEvent::eTraceEventNone if the cursor not pointing to an event.
+  lldb::TraceEvent GetEventType() const;
----------------
wallace wrote:
> The event eTraceEventNone was deleted, so don't mention it here.
thanks - this is copypasta from TraceCursor.h, so I'll go ahead and fix this here and there


================
Comment at: lldb/source/API/SBTrace.cpp:47
+SBTraceCursor SBTrace::CreateNewCursor(SBError &error, SBThread &thread) {
+  LLDB_INSTRUMENT_VA(this);
+  if (!m_opaque_sp || !thread.get()) {
----------------
JDevlieghere wrote:
> There should be a newline after `LLDB_INSTRUMENT_VA` to match the output of `lldb-instr`. 
thanks for catching this!


================
Comment at: lldb/source/API/SBTraceCursor.cpp:17-18
+
+// relevant to the clone issue:
+// https://stackoverflow.com/questions/16030081/copy-constructor-for-a-class-with-unique-ptr
+SBTraceCursor::SBTraceCursor() { LLDB_INSTRUMENT_VA(this); }
----------------
wallace wrote:
> no need to mention this. You can just create a invalid SBTraceCursor. Don't forget to add the IsValid method
oops I added this as a reference when I was exploring adding the bindings before changing to using SP and forgot to delete it. will remove - thanks!


================
Comment at: lldb/source/API/SBTraceCursor.cpp:63-64
+  LLDB_INSTRUMENT_VA(this, offset, origin);
+  // Convert from public `SBTraceCursor::SeekType` enum to private
+  // `TraceCursor::SeekType` enum.
+  auto convert_seek_type_enum = [](SeekType seek_type) {
----------------
wallace wrote:
> You should instead create a new public enumeration TraceCursorSeekType. That will simplify the code
I was leaning towards this while adding the bindings, so yea will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130930