[Lldb-commits] [PATCH] D105531: [trace][intel pt] Implement the Intel PT cursor

[Greg Clayton via Phabricator via lldb-commits]

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

Very close! Just a few nits.



================
Comment at: lldb/include/lldb/Target/TraceCursor.h:101
+  /// Set the direction to use in the \a TraceCursor::Move() method.
+  void SetDirection(Direction direction);
+
----------------
If there is no other direction other than forward to backward, should this just be a bool? Then we won't have to add the enum to the public API when we expose this.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:105
+  ///     The current direction of the cursor.
+  TraceCursor::Direction GetDirection();
+
----------------
bool? Fine if you think the enum should be used.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:134-135
+  ///     The number of matching instructions visited before stopping the
+  ///     cursor. If stopped at an error, this count is increased by 1 to
+  ///     account for the error.
+  virtual size_t Move(size_t count = 1) = 0;
----------------
If we get an error, we should return less than the requested amount. If you ask for 10, and only get 7, but get an error, you don't want to return 8. So I would vote for documentation and implementation like:
```
The number of successful moves that occurred. If the returned value is less than \a count, then check for an error condition by calling XXX. If there is no error and less than \a count was returned, then the cursor is at the end or beginnning of the data.
```


================
Comment at: lldb/source/Target/TraceCursor.cpp:20-35
+ExecutionContextRef &TraceCursor::GetExecutionContextRef() {
+  return m_exe_ctx_ref;
+}
+
+void TraceCursor::SetGranularity(
+    lldb::TraceInstructionControlFlowType granularity) {
+  m_granularity = granularity;
----------------
All these can be moved to the header file. We don't do this in the public API, but we can do this in private classes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105531