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

[Jan Korous via Phabricator via cfe-commits]

jkorous added reviewers: ilya-biryukov, ioeric.
jkorous added a comment.

Since AFAIK Sam is off until the end of the year I am adding more reviewers.



================
Comment at: tool/ClangdMain.cpp:329
+    if (getenv("CLANGD_AS_XPC_SERVICE"))
+      return newXPCransport();
+#endif
----------------
jkorous wrote:
> sammccall wrote:
> > no support for `-input-mirror` or pretty-printing in the logs - deliberate?
> I was thinking this could be added later.
Just to clarify - since we'd like to have just a single binary for XPC and stdio transport and we sadly can't use command line options we'd have to duplicate probably all of them as env variables.
Would that be fine with you?


================
Comment at: xpc/CMakeLists.txt:21
+
+add_clang_library(clangdXpcJsonConversions
+  Conversion.cpp
----------------
jkorous wrote:
> sammccall wrote:
> > 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.
> Guilty of YAGNI violation here - was thinking about eventual use by XPC clients. I'll stick to consistency for now and would separate it if needed.
Now that I actually took a better look we already have one client - our xpc-test-client. I assume that means - it's fine as it is, right?


================
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));
----------------
jkorous wrote:
> sammccall wrote:
> > 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?
> Our use-case is an XPC service which is bundled to an application. It could be described as a "private daemon" - the service is launched on-demand for the application. There should be either zero or one connection at a time.
> 
> As for messages - we didn't plan for concurrent use. I'll add explicit synchronization.
I want to make the synchronous operation explicit here. Still need to work on this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54428/new/

https://reviews.llvm.org/D54428