jkorous added a reviewer: sammccall. jkorous added a comment. Hi Sam, I am definitely open to discussion about the right abstraction.
I will push patches rebased on TOT and changes based on your comments today or tomorrow and I am trying to figure out how could we use your Transport abstraction proposal (https://reviews.llvm.org/D49389) without Json in it's interface in the meantime. ================ Comment at: Features.inc.in:1 +#define CLANGD_ENABLE_XPC_SUPPORT @CLANGD_BUILD_XPC_SUPPORT@ ---------------- sammccall wrote: > nit: can we give these the same name? > I'd vote for dropping the `_SUPPORT` too: either `CLANGD_ENABLE_XPC` or > `CLANGD_BUILD_XPC`. > > Maybe the latter is better, since an environment variable controls the > runtime behavior anyway. Good point. I will change that. ================ Comment at: tool/ClangdMain.cpp:261 +#ifdef CLANGD_ENABLE_XPC_SUPPORT + if (getenv("CLANGD_AS_XPC_SERVICE")) { + XPCDispatcher Dispatcher([](const json::Expr &Params) { ---------------- sammccall wrote: > is there a reason this is an environment variable rather than an `-xpc` flag? > > Apart from this being (mostly) what we do elsewhere, it seems like if the > process spawning clangd wants XPC suppport, but clangd was built without it, > then you want the process to fail and exit rather than sit there listening on > stdin. Flags have this behavior, env variables do not. I agree that flag would be a natural choice here (and was my original intention) but unfortunately Launchd doesn't support that for spawning bundled XPC services. Do you mean I should check for the env variable also in case clangd is not built with XPC support and conditionally exit? ================ Comment at: tool/ClangdMain.cpp:262 + if (getenv("CLANGD_AS_XPC_SERVICE")) { + XPCDispatcher Dispatcher([](const json::Expr &Params) { + replyError(ErrorCode::MethodNotFound, "method not found"); ---------------- sammccall wrote: > the duplication here suggests to me the factoring isn't quite right. > > naively, ISTM we should be able to have two implementations of an interface > here rather than an actual ifdef'd server loop. But I'll dig into the > implementation to try to understand a bit more. I totally understand your thoughts - I originally started with the same idea in the end wasn't able to implement it in simple enough way. Ultimately I decided to prefer simplicity over nice abstraction. But I am happy to discuss this and make changes accordingly. https://reviews.llvm.org/D48562 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
