[PATCH] D39086: Performance tracing facility for clangd.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Looks good in general. A few nits.



================
Comment at: clangd/JSONRPCDispatcher.cpp:230
 
-      // Finally, execute the action for this JSON message.
-      if (!Dispatcher.call(JSONRef, Out))
-        Out.log("JSON dispatch failed!\n");
+      {
+        // Finally, execute the action for this JSON message.
----------------
Why this empty block?


================
Comment at: clangd/Trace.cpp:90
+
+void start(raw_ostream &OS) {
+  assert(!T && "start() called twice without stop()");
----------------
Would it make sense to have a helper class which handles starting/stopping in, say, constructor/destructor? ... so that we don't need to worry about forgetting to stop?


================
Comment at: clangd/tool/ClangdMain.cpp:98
+      llvm::errs() << "Error while opening trace file: " << EC.message();
+    }
+    trace::start(*TraceStream);
----------------
Is this intended? Start tracing even if there is an error?


================
Comment at: unittests/clangd/TraceTests.cpp:47
+    auto I = Expected.find(KS);
+    if (I == Expected.end()) continue;
+
----------------
Add a comment on why we skip unexpected keys?


https://reviews.llvm.org/D39086