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

[walter erquinigo via Phabricator via lldb-commits]

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

pretty nice!! Just some few minor changes and good to go



================
Comment at: lldb/bindings/interface/SBTraceCursor.i:1-7
+//===-- SWIG Interface for SBTraceCursor.h ----------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
----------------
the first and last lines should have 80 cols


================
Comment at: lldb/bindings/interface/SBTraceCursor.i:61
+
+  lldb::cpu_id_t GetCPU(SBError &error) const;
+};
----------------
add an IsValid method. It's common in the SB API to offer such method as an alternative to heck the error object.


================
Comment at: lldb/include/lldb/API/SBTrace.h:38-39
+  /// \return
+  ///     A \a SBTraceCursor. If the thread is not traced or its trace
+  ///     information failed to load, an \a llvm::Error is returned.
+  SBTraceCursor CreateNewCursor(SBError &error, SBThread &thread);
----------------



================
Comment at: lldb/include/lldb/API/SBTraceCursor.h:1-2
+//===-- SBTraceCursor.h -----------------------------------------------*- C++
+//-*-===//
+//
----------------
JDevlieghere wrote:
> Broken ASCII art
fix this


================
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;
----------------
The event eTraceEventNone was deleted, so don't mention it here.


================
Comment at: lldb/include/lldb/API/SBTraceCursor.h:181-182
+
+  // TODO: should we define LLDB_INVALID_CPU_ID so this matches the behavior of
+  // `GetLoadAddress()`?
+  lldb::cpu_id_t GetCPU(SBError &error) const;
----------------
yes, you should. And you can set it to -1


================
Comment at: lldb/source/API/SBTrace.cpp:48-51
+  if (!m_opaque_sp || !thread.get()) {
+    error.SetErrorString("error: invalid trace");
+    return SBTraceCursor();
+  }
----------------
you should have one error for invalid thread and one for invalid trace


================
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); }
----------------
no need to mention this. You can just create a invalid SBTraceCursor. Don't forget to add the IsValid method


================
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) {
----------------
You should instead create a new public enumeration TraceCursorSeekType. That will simplify the code


================
Comment at: lldb/source/API/SBTraceCursor.cpp:119-120
+
+// TODO: should we define LLDB_INVALID_CPU_ID so this matches the behavior of
+// `GetLoadAddress()`?
+lldb::cpu_id_t SBTraceCursor::GetCPU(SBError &error) const {
----------------
yes, define a LLDB_INVALID_CPU_ID and don't return an error here


================
Comment at: lldb/test/API/commands/trace/TestTraceLoad.py:342-346
+                
+
+
+
+
----------------
add one more test that simply walks through a few items expecting some hardcoded values. That kind of test are easier to fix when something minor breaks. The other kind of tests, like the one you just wrote above, are more complicated to fix but they are much stronger test. Both kinds of tests are useful


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130930