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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
