sammccall added a comment. Hi Jan,
Thanks for putting this together, and apologies - this is one of the places where we don't have nice abstractions/layering, so adding XPC was harder than it should be. As I mentioned on the other review, I think maybe this patch isn't invasive enough, we could do a more thorough job of making the json transport a standalone, first-class thing. This makes it easier to swap out for XPC but also would improve the layering in the core clangd classes. I put together a sketch of a `Transport` interface in https://reviews.llvm.org/D49389 (that patch is extremely unfinished and won't compile, but the idea might be interesting). The idea is that we should be able to implement that class for XPC and then it should just drop-in as a replacement for the JSON one. I've indicated there where XPC and JSON implementation can share code (assuming you wouldn't rather "tighten up" the protocol and eliminate some JSON-RPC noise). If you like this direction I'm happy for you to pick the bits you like, or I can finish it as a refactoring and we can make sure it works for XPC. If not, that's fine too - happy to look at other ways to reduce duplication between them. ================ Comment at: DispatcherCommon.h:38 + // Return a context that's aware of the enclosing request, identified by Span. + static Context stash(const trace::Span &Span); + ---------------- I think I wrote this bit... it's a hack that I'm not sure deserves to be visible in a header (it was bad enough in JSONRPCDispatcher.cpp!) Rather than exposing it so we can use it twice, I'd hope we can manage to keep it hidden so that JSON and XPC can share it. 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
