[PATCH] D48559: [clangd] refactoring for XPC transport layer [NFCI]

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D48559#1165172, @jkorous wrote:

> Hi Sam, thanks for your feedback!
>
> In general I agree that explicitly factoring out the transport layer and improving layering would be a good thing. Unfortunately it's highly probable that we'd like to drop JSON completely from XPC dispatch (XPC -> JSON -> ProtocolCallbacks -> JSON -> XPC) in not too distant future. (Please don't take this as a promise as our priorities might change but it's probable enough that I don't recommend to base any common abstraction on JSON.) I originally tried to create similar abstraction but ultimately gave up as it was way too complex. Not saying it's impossible or that I would not like to though!


That makes sense and is good to know. I definitely don't want to add a transport abstraction that isn't a good fit for your long-term plans.

I think we need to clearly see what the goal is to get the design right though - it doesn't make sense to refactor in order to share code temporarily. Let's take code completion as an example, as it's one of the ones we've fleshed out most for our internal (non-LSP) clients.

- if the goal is to mirror JSON-RPC with a different encoding (current patches), and you're just concerned about the *efficiency* of `CodeCompletion` -> `CompletionItem` -> `json::Value` -> `xpc_object_t`, this seems like premature optimization to me, but let's discuss!
- if the goal is to basically follow the LSP (same textDocument/completion for), using the `CompletionItem` structs in `Protocol.h` but exposing more/fewer fields, renaming them etc, then we should probably have the feature code express replies as protocol structs, and make the transport define a way to transform specific objects (CompletionItem) into generic ones whose type (json::Value or xpc_object_t) are transport-specific
- if the goal is to maintain a stable protocol that can be evolved independently of LSP (different methods or semantics) then this isn't really a transport, it's a new protocol. I think the best option is probably to skip `ClangdLSPServer` and have the xpc service wrap `ClangdServer` instead, with a separate main entrypoint. The litmus test here: as people make changes to clangd that affect the LSP surface, do you want the XPC service's interface to change too? This is the hardest model to support as the C++ API (ClangdServer) is not a stable one, but it's what we do for the other non-LSP clients.

Does one of these seem close to what you want? Any thoughts on the conclusions?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48559