This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Lay JSONRPCDispatcher to rest.
ClosedPublic

Authored by sammccall on Oct 17 2018, 3:14 PM.

Details

Summary

Most of its functionality is moved into ClangdLSPServer.
The decoupling between JSONRPCDispatcher, ProtocolCallbacks, ClangdLSPServer
was never real, and only served to obfuscate.

Some previous implicit/magic stuff is now explicit:

  • the return type of LSP method calls are now in the signature
  • no more reply() that gets the ID using global context magic
  • arg tracing no longer relies on RequestArgs::stash context magic either

This is mostly refactoring, but some deliberate fixes while here:

  • LSP method params are now by const reference
  • notifications and calls are now distinct namespaces. (some tests had protocol errors and needed updating)
  • we now reply to calls we failed to decode
  • outgoing calls use distinct IDs

A few error codes and message IDs changed in unimportant ways (see tests).

Diff Detail

Event Timeline

sammccall created this revision.Oct 17 2018, 3:14 PM
ioeric accepted this revision.Oct 18 2018, 5:13 AM

lgtm

clangd/ClangdLSPServer.cpp
148

do we really want to stash all responses in trace? It sounds like it could get pretty spammy (e.g. with completion results).

184

nit: while we are here, could you add some documentation for these two fields?

409

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.

clangd/ClangdLSPServer.h
34

Just out of curiosity, why does DiagnosticsConsumer implemented in ClangdLSPServer instead of ClangdServer?

This revision is now accepted and ready to land.Oct 18 2018, 5:13 AM
sammccall marked an inline comment as done.Oct 18 2018, 5:32 AM
sammccall added inline comments.
clangd/ClangdLSPServer.cpp
148

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.

409

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).

clangd/ClangdLSPServer.h
34

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.

This revision was automatically updated to reflect the committed changes.
clangd/ProtocolHandlers.h