[PATCH] D39086: Performance tracing facility for clangd.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
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.
----------------
ioeric wrote:
> Why this empty block?
Oops, was tracing here but it didn't turn out to be useful. Reverted.


================
Comment at: clangd/Trace.cpp:90
+
+void start(raw_ostream &OS) {
+  assert(!T && "start() called twice without stop()");
----------------
ioeric wrote:
> 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?
I didn't do this for a couple of reasons:

 - it obscures the static-stateful nature of this code, making it look like e.g. you could dynamically turn on tracing while clangd is running. (Which would be cool, but wouldn't currently work)
 - it's some more boilerplate, and isn't actually very convenient to ClangdMain (since `start()` is conditional)


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


https://reviews.llvm.org/D39086