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

[Pavel Labath via Phabricator via lldb-commits]

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