[PATCH] D53387: [clangd] Lay JSONRPCDispatcher to rest.

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: clangd/ClangdLSPServer.cpp:148
+          if (Trace)
+            (*Trace)["Reply"] = *Result;
+          Server.reply(ID, json::Value(std::move(*Result)));
----------------
do we really want to stash all responses in trace? It sounds like it could get pretty spammy (e.g. with completion results). 


================
Comment at: clangd/ClangdLSPServer.cpp:184
+  mutable std::mutex RequestCancelersMutex;
+  llvm::StringMap<std::pair<Canceler, unsigned>> RequestCancelers;
+  unsigned NextRequestCookie = 0;
----------------
nit: while we are here, could you add some documentation for these two fields?


================
Comment at: clangd/ClangdLSPServer.cpp:409
+
+            Reply(std::move(*Items));
+          },
----------------
We are not translating internal errors from ClangdServer to LSP errors anymore. It seems fine and makes code cleaner. Just want to ask if this is intentional.


================
Comment at: clangd/ClangdLSPServer.h:34
+/// The server also supports $/cancelRequest (MessageHandler provides this).
+class ClangdLSPServer : private DiagnosticsConsumer {
 public:
----------------
Just out of curiosity, why does `DiagnosticsConsumer` implemented in ClangdLSPServer instead of ClangdServer? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53387