[Lldb-commits] [PATCH] D88841: [intel pt] Refactor parsing

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/include/lldb/Core/PluginManager.h:342
 
+  static const char *GetTraceSchema(ConstString name);
+
----------------
This is fine if we have a case where we want the scheme for a given plug-in. We should probably also add a call that takes a "size_t index" as an argument that is the index of the plug-ins. When the index is too big, returns NULL. This will allow us to iterate over all of the installed plug-ins and print out the schemas for each one?


================
Comment at: lldb/include/lldb/Target/Trace.h:94
+  static llvm::Expected<llvm::StringRef>
+  FindPluginSchema(llvm::StringRef plugin_name);
 
----------------
Do we also want a non static version of this to be able to get the schema for the current Trace plugin? 


================
Comment at: lldb/include/lldb/Target/TraceSessionFileParser.h:107
+
+using namespace lldb_private;
+
----------------
labath wrote:
> delete. "using namespace" in a header file is a big no-no.
indeed! I will trickly through to any .cpp file that includes it


================
Comment at: lldb/source/Commands/CommandObjectTrace.cpp:242
                             "Show the schema of the given trace plugin.",
                             "trace schema <plug-in>"),
         m_options() {}
----------------
Make <plug-in> optional so we can list all trace plug-in schemas?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h:14
+
+class ThreadIntelPT : public lldb_private::Thread {
+public:
----------------
labath wrote:
> btw, (most of) new plugins tend to place themselves in a sub-namespace of lldb_private. That avoids prefixing everything with `lldb_private::`
I second this, add a plug-in namespace!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88841