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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/bindings/interface/SBTraceCursor.i:56
+
+  explicit operator bool() const;
+};
----------------
+1


================
Comment at: lldb/include/lldb/API/SBTraceCursor.h:21
+public:
+  /// Default constructor for an invalid \a SBTraceCursor object.
+  SBTraceCursor();
----------------
+1


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:201
 lldb::TraceEvent DecodedThread::GetEventByIndex(int item_index) const {
+  // This currently returns an undefined value when the item isn't an event.
   return m_item_data[item_index].event;
----------------
jj10306 wrote:
> what's the best return value if this is called and the item isn't an event?
> ideally we could return something similar to LLDB_INVALID_ADDRESS, but since this is an enum we would need to add a new variant that represents the "invalid" case. Perhaps we could bring back the `eTraceEventNone` variant that was recently removed?
> wdyt?
I was thinking about that and I think a better contract is to expect users not to use a getter that is unrelated to the current trace type. If we don't add strict expectations, then consumers might create buggy code without realizing.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:234-236
+  return GetItemKindByIndex(item_index) == lldb::eTraceItemKindError
+             ? m_item_data[item_index].error
+             : nullptr;
----------------
like this. It might be better just to fail or return garbage and that will let the user know that they are doing something they shouldn't. 

Besides that, in c++ it's easy to look for nullptr when getting char *, but in python, this might be converted eventually to an actual empty str, and that might hide some possible bugs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130930