[PATCH] D54428: [clangd] XPC transport layer, framework, test-client

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

A question about the high-level build target setup (I don't know much about XPC services/frameworks, bear with me...):

This is set up so that the clangd binary (ClangdMain) can run unmodified as an XPC service, all flags and options are still respected etc.
At the same time, the framework plist seems like it is designed to invoke the clangd binary in a very specific configuration (no flags will be plumbed through).

Is it actually important that the `clangd` binary itself can be used with XPC, rather than defining another binary that spawns a `ClangdServer+XPCTransport` with the right config? If you strip out all of `ClangdMain` that's not related to flag parsing and options, there's almost nothing left...



================
Comment at: tool/ClangdMain.cpp:30
+#ifdef CLANGD_BUILD_XPC
+#include "xpc/XPCTransport.h"
+#endif
----------------
WDYT about putting `newXPCTransport()` in `Transport.h` behind an ifdef?

That will be consistent with JSON, allow the `XPCTransport.h` to be removed entirely, and remove one #ifdef here.

I find the #ifdefs easier to understand if they surround functional code, rather than #includes - might be just me.


================
Comment at: tool/ClangdMain.cpp:328
+#ifdef CLANGD_BUILD_XPC
+    if (getenv("CLANGD_AS_XPC_SERVICE"))
+      return newXPCransport();
----------------
I'd consider putting this outside the #ifdef, so we can print an error and exit if it's requested but not built.

In fact, can this just be a regular flag instead? `-transport={xpc|json}`


================
Comment at: tool/ClangdMain.cpp:329
+    if (getenv("CLANGD_AS_XPC_SERVICE"))
+      return newXPCransport();
+#endif
----------------
no support for `-input-mirror` or pretty-printing in the logs - deliberate?


================
Comment at: xpc/CMakeLists.txt:21
+
+add_clang_library(clangdXpcJsonConversions
+  Conversion.cpp
----------------
why is conversions a separate library from transport?

No problem with fine-grained targets per se, but it's inconsistent with much of the rest of clang-tools-extra.


================
Comment at: xpc/Conversion.cpp:16
+
+using namespace clang;
+using namespace clangd;
----------------
nit:
```
using namespace llvm;
namespace clang {
namespace clangd {
```
(we're finally consistent about this)


================
Comment at: xpc/Conversion.cpp:22
+xpc_object_t clangd::jsonToXpc(const json::Value &json) {
+  const char *const key = "LSP";
+  std::string payload_string;
----------------
Key, PayloadString, etc


================
Comment at: xpc/Conversion.cpp:23
+  const char *const key = "LSP";
+  std::string payload_string;
+  raw_string_ostream payload_stream(payload_string);
----------------
nit: you can #include ScopedPrinter, and then this is just llvm::to_string(json)


================
Comment at: xpc/Conversion.cpp:28
+  xpc_object_t payload_obj = xpc_string_create(payload_string.c_str());
+  return xpc_dictionary_create(&key, &payload_obj, 1);
+}
----------------
ah, this encoding is a little sad vs the "native" encoding of object -> xpc_dictionary, array -> xpc_array etc which looked promising...

Totally your call, but out of curiosity - why the change?


================
Comment at: xpc/Conversion.h:19
+
+xpc_object_t jsonToXpc(const llvm::json::Value &json);
+llvm::json::Value xpcToJson(const xpc_object_t &json);
----------------
param name json -> JSON


================
Comment at: xpc/XPCTransport.cpp:142
+
+namespace XPCClosure {
+// "owner" of this "closure object" - necessary for propagating connection to
----------------
lowercase namespace?


================
Comment at: xpc/XPCTransport.cpp:177
+    if (!TransportObject->handleMessage(std::move(Doc), *HandlerPtr)) {
+      log("Received exit notification - cancelling connection.");
+      xpc_connection_cancel(xpc_dictionary_get_remote_connection(message));
----------------
I'm not clear on the relationship between XPC services and connections - if there are multiple connections, what happens? Do you want to check and close any subsequent ones? Can connections and/or messages arrive on multiple threads?


================
Comment at: xpc/XPCTransport.cpp:192
+  // exit of xpc_main().
+  XPCClosure::TransportObject = this;
+  XPCClosure::HandlerPtr = &Handler;
----------------
you could consider asserting that they were previously null


================
Comment at: xpc/XPCTransport.h:19
+
+std::unique_ptr<Transport> newXPCransport();
+
----------------
as mentioned above, I think this may be more discoverable in Transport.h, with ifdef.
But if the layering/separate libraries are important, here seems OK too.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54428