labath added a comment.
I like this. A lot of comments, but they're all cosmetic.
================
Comment at: lldb/include/lldb/Target/Trace.h:96-98
protected:
Trace() {}
----------------
delete completely. As this is an abstract class, making the constructor
protected does not do anything.
================
Comment at: lldb/include/lldb/Target/TraceSessionFileParser.h:9
+
+#ifndef LLDB_TARGET_TRACE_SESSION_PARSER_H
+#define LLDB_TARGET_TRACE_SESSION_PARSER_H
----------------
I think it wants this to be `LLDB_TARGET_TRACESESSIONPARSER_H`
================
Comment at: lldb/include/lldb/Target/TraceSessionFileParser.h:107
+
+using namespace lldb_private;
+
----------------
delete. "using namespace" in a header file is a big no-no.
================
Comment at: lldb/include/lldb/Target/TraceSessionFileParser.h:109
+
+inline bool fromJSON(const Value &value,
+ TraceSessionFileParser::JSONAddress &address, Path path);
----------------
delete inline
================
Comment at: lldb/source/Commands/CommandObjectTrace.cpp:263
+ else
+ error.SetErrorString(llvm::toString(schemaOrErr.takeError()));
----------------
`error = schemaOrErr.takeError()`
================
Comment at: lldb/source/Core/PluginManager.cpp:1043
+const char *PluginManager::GetTraceSchema(ConstString name) {
+ for (const TraceInstance &instance : GetTraceInstances().GetInstances())
----------------
Return StringRef
================
Comment at: lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.cpp:23-25
+ if (!m_reg_context_sp) {
+ m_reg_context_sp = CreateRegisterContextForFrame(nullptr);
+ }
----------------
[[
http://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements|
no braces on simple if bodies ]]
================
Comment at: lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h:14
+
+class ThreadIntelPT : public lldb_private::Thread {
+public:
----------------
btw, (most of) new plugins tend to place themselves in a sub-namespace of
lldb_private. That avoids prefixing everything with `lldb_private::`
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:23
+ GetPluginNameStatic(), "Intel Processor Trace", CreateInstance,
+ TraceIntelPTSessionFileParser::GetSchema().data());
}
----------------
drop `data()` and have this function take a StringRef ?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:46
+lldb::TraceSP
+TraceIntelPT::CreateInstance(const llvm::json::Value &trace_session_file,
+ llvm::StringRef session_file_dir,
----------------
This could probably also return an `Expected<TraceSP>`
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:64
+ : Trace(), m_pt_cpu(pt_cpu) {
+ for (auto target_sp : targets)
+ m_targets.push_back(target_sp);
----------------
[[
http://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto |
const auto & ]], or potentially even `const TargetSP &` due to [[
http://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto |
this ]].
================
Comment at:
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:18
+using namespace lldb_private;
+using namespace llvm;
+
----------------
You're `using namespace llvm`, but then still prefixing a lot of the stuff with
`llvm::`. Is that because `llvm::json::` is a mouthful? `namespace json =
llvm::json`, maybe?
================
Comment at:
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h:80
+bool fromJSON(const Value &value,
+ TraceIntelPTSessionFileParser::JSONPTCPU &pt_cpu, Path path);
+
----------------
This demonstrates why the previous "using namespace" is so bad. Now the
entirety of lldb is accessible from within the llvm::json namespace. :D
================
Comment at: lldb/source/Target/TraceSessionFileParser.cpp:98
+// Notes:
+// All paths are either absolute or relative to the session file.)";
+ return schema_builder.str();
----------------
End this with a newline ?
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
