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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.cpp:20
+
+ThreadIntelPT::~ThreadIntelPT() {}
+
----------------
just `=default`, or even don't declare it all (as explained in the other thread, these declarations are completely redundant).


================
Comment at: lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h:9-10
+
+#ifndef liblldb_ThreadIntelPT_H
+#define liblldb_ThreadIntelPT_H
+
----------------
Yep, all our header guards have been renamed now. Please don't reintroduce the old style.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.cpp:167
+
+  return TraceSP(new TraceIntelPT(m_pt_cpu, m_targets));
+}
----------------
std::make_shared


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.h:21-59
+  struct JSONAddress {
+    lldb::addr_t value;
+  };
+
+  struct JSONModule {
+    std::string system_path;
+    llvm::Optional<std::string> file;
----------------
clayborg wrote:
> Seems like we should still parse this in Trace.cpp. Any common information shouldn't be duplicated or represented differently in different trace plug-ins IMHO. 
How much of this stuff needs to be in the header? If parsing happens completely within this class, I'd expect that these classes (and the relevant fromJSON methods) could be in the cpp file (in an anon namespace).


================
Comment at: lldb/source/Target/Trace.cpp:80-82
+    TraceSP instance = create_callback(/*trace_session_file*/ nullptr,
+                                       /*session_file_dir*/ nullptr,
+                                       /*debugger*/ nullptr, error);
----------------
clayborg wrote:
> Why have the create_callback take an of these arguments if they are always null?
It seems this is used to implement "trace schema". I think that a better way to implement this functionality would be to have each plugin pass its schema (as a string) when it registers itself (in TraceXXX::Initialize`). Then we can have an entry point like `StringRef Trace::GetSchema(StringRef plugin_name)` which will return that, and there won't be a need to create a polymorphic object just for the sake of calling it GetSchema on it (you can still have one if you need to get the schema for an existing object for any reason).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88841