arphaman added a comment. In https://reviews.llvm.org/D48559#1165511, @sammccall wrote:
> 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? Hi Sam, Thanks for your feedback! Generally right now will be mirroring JSON-RPC with a different encoding, but in the future we will send some optimized payloads for a subset of replies which are constructed out of things like `CompletionItem` directly (so bypassing the JSON step). I believe that your patch (https://reviews.llvm.org/D49389) is implementing the JSON-RPC with a choice of encoding, which will work for us right now, and it doesn't look like we will have problems optimizing our payloads later when we choose to do so. However, the second option that you proposed does sound quite compelling as well (i.e. `then we should probably have the feature code express replies as protocol structs`), as it would give us more flexibility. However, I think it's orthogonal to the first option (JSON-RPC with a choice of encoding), so maybe we can leave it as something that will be on the table if we need it later? I think for the transport layer idea you proposed in https://reviews.llvm.org/D49389 makes sense and will work for us, and in the future we always can implement the second option that your proposed on top of of your patch, or use a simpler way to optimize our payloads without much disruption. Thanks, Alex Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48559 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
