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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:148
+          if (Trace)
+            (*Trace)["Reply"] = *Result;
+          Server.reply(ID, json::Value(std::move(*Result)));
----------------
ioeric wrote:
> do we really want to stash all responses in trace? It sounds like it could get pretty spammy (e.g. with completion results). 
It's a fair point, I don't know the answer. I know I've looked at the replies, but not all that often.

It's not new behavior though (it used to be in JSONRPCDispatcher), and I'd rather not change it in this patch.


================
Comment at: clangd/ClangdLSPServer.cpp:409
+
+            Reply(std::move(*Items));
+          },
----------------
ioeric wrote:
> 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.
We used to explicitly mark some of these with the "internal error" code, and others with the "unknown error" code, without much consistency as to why.

I've tried to preserve the error codes that were semantically significant (e.g. invalid params).


================
Comment at: clangd/ClangdLSPServer.h:34
+/// The server also supports $/cancelRequest (MessageHandler provides this).
+class ClangdLSPServer : private DiagnosticsConsumer {
 public:
----------------
ioeric wrote:
> Just out of curiosity, why does `DiagnosticsConsumer` implemented in ClangdLSPServer instead of ClangdServer? 
This is `clangd::DiagnosticsConsumer`, it's exactly the callback that ClangdServer uses to send diagnostics to ClangdLSPServer.

So ClangdServer doesn't implement it but rather accepts it, because it doesn't know what to do with the diagnostics.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53387