[PATCH] D48562: [clangd] XPC transport layer

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Just an initial couple of thoughts here, haven't yet been through in detail.

Mostly I wonder if we can use slightly different abstractions in https://reviews.llvm.org/D48559 to so the JSON/XPC parts get the code reuse we want but the work required to call one vs the other is smaller. Need to think about this more.



================
Comment at: Features.inc.in:1
+#define CLANGD_ENABLE_XPC_SUPPORT @CLANGD_BUILD_XPC_SUPPORT@
----------------
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.


================
Comment at: tool/ClangdMain.cpp:261
+#ifdef CLANGD_ENABLE_XPC_SUPPORT
+  if (getenv("CLANGD_AS_XPC_SERVICE")) {
+    XPCDispatcher Dispatcher([](const json::Expr &Params) {
----------------
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.


================
Comment at: tool/ClangdMain.cpp:262
+  if (getenv("CLANGD_AS_XPC_SERVICE")) {
+    XPCDispatcher Dispatcher([](const json::Expr &Params) {
+      replyError(ErrorCode::MethodNotFound, "method not found");
----------------
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.


https://reviews.llvm.org/D48562