[PATCH] D48562: [clangd] XPC transport layer

[Jan Korous via Phabricator via cfe-commits]

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