[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

Took a look at the SBAPI changes and left few comments but overall, that part looked good to me. It would be nice if you could delete the depreciated method/class.



================
Comment at: lldb/bindings/interface/SBTrace.i:32-51
+  /// deprecated
   void StopTrace(SBError &error,
                  lldb::tid_t thread_id);
 
+  /// deprecated
   void GetTraceConfig(SBTraceOptions &options,
                       SBError &error);
----------------
JDevlieghere wrote:
> Do you have clients that already rely on these functions? Writing deprecated above them means nothing. When I was doing the reproducers, I included a warning in the header saying that this was under development and the API wasn't final, which allowed me to iterate on it. Can we just remove these methods and to the same here? 
+1


================
Comment at: lldb/bindings/interface/SBTraceOptions.i:12
 %feature("docstring",
-"Represents the possible options when doing processor tracing.
-
-See :py:class:`SBProcess.StartTrace`."
+"deprecated"
 ) SBTraceOptions;
----------------
Same comment as @JDevlieghere here


================
Comment at: lldb/include/lldb/API/SBTrace.h:100
 
-protected:
-  typedef std::shared_ptr<TraceImpl> TraceImplSP;
-
-  friend class SBProcess;
+  /// Deprecated
+  /// \{
----------------
Same here.


================
Comment at: lldb/source/API/SBTrace.cpp:92
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBTrace, IsValid);
-  return this->operator bool();
+  return LLDB_RECORD_RESULT(this->operator bool());
 }
----------------
No need to use `LLDB_RECORD_RESULT` on primitive types such as `bool`.


================
Comment at: lldb/source/API/SBTrace.cpp:97
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBTrace, operator bool);
+  return LLDB_RECORD_RESULT((bool)m_opaque_sp);
+}
----------------
Same here.


================
Comment at: lldb/source/API/SBTrace.cpp:103
+                             size_t offset, lldb::tid_t thread_id) {
+  LLDB_RECORD_DUMMY(size_t, SBTrace, GetTraceData,
+                    (lldb::SBError &, void *, size_t, size_t, lldb::tid_t),
----------------
Either use `LLDB_RECORD_DUMMY` on all the deprecated method or don't use it at all :) If you could get rid of the deprecated methods that would be even better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500