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