[PATCH] D48560: [clangd] JSON <-> XPC conversions

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Sorry it took so long for these to get attention.
Starting here because it's simple and helps me understand the bigger patches....
This one looks good, really just nits.



================
Comment at: clangd/xpc/CMakeLists.txt:1
+set(LLVM_LINK_COMPONENTS
+  support
----------------
I think it'd be helpful for some of us confused linux people to have a brief README in this directory giving some context about XPC in clangd.

Anything you want to write about the architecture is great but the essentials would be:
 - alternative transport layer to JSON-RPC
 - semantically equivalent, uses same LSP request/response message types (?)
 - MacOS only. Feature is guarded by `CLANGD_BUILD_XPC_SUPPORT`, including whole `xpc/` dir.


================
Comment at: clangd/xpc/CMakeLists.txt:15
+
+# FIXME: Factor out json::Expr from clangDaemon
+target_link_libraries(ClangdXpcTests
----------------
this is done :-)


================
Comment at: xpc/XPCJSONConversions.cpp:22
+    case json::Expr::Boolean: return xpc_bool_create(json.asBoolean().getValue());
+    case json::Expr::Number:  return xpc_double_create(json.asNumber().getValue());
+    case json::Expr::String:  return xpc_string_create(json.asString().getValue().str().c_str());
----------------
The underlying JSON library actually has both `int64_t` and `double` representations, and I see XPC has both as well.
If `double` works for all the data sent over this channel, then this approach is simplest. But for completeness, this is also possible:
```
if (auto I = json.asInteger())
  return xpc_int64_create(*I);
return xpc_double_create(*json.asNumber());
```

(I don't know of any reason clangd would use large integers today unless they arrived as incoming JSON-RPC request IDs or similar)


================
Comment at: xpc/XPCJSONConversions.cpp:40
+      }
+      // Get pointers only now so there's no possible re-allocation of keys.
+      for(const auto& k : keys)
----------------
(you could also pre-size the vector, or use a deque)


================
Comment at: xpc/XPCJSONConversions.cpp:62
+  if (objType == XPC_TYPE_UINT64)
+    return json::Expr(xpc_uint64_get_value(xpcObj));
+  if (objType == XPC_TYPE_STRING)
----------------
hmm, I think this shouldn't compile anymore, rather require you to explicitly do the narrowing conversion to int64 or double.


================
Comment at: xpc/XPCJSONConversions.cpp:69
+      xpcObj,
+      ^bool(size_t /* index */, xpc_object_t value) {
+        result.emplace_back(xpcToJson(value));
----------------
this looks like objC syntax, I'm not familiar with it, but should this file be `.mm`?

Alternatively, it seems like you could write a C++ loop with `xpc_array_get_value` (though I don't know if there are performance concerns).

(and dictionary)


================
Comment at: xpc/XPCJSONConversions.cpp:87
+  }
+  llvm_unreachable("unsupported JSON data type in xpcToJson() conversion");
+}
----------------
there are other data types (error, fd, ...) and this data presumably comes over a socket or something, so you may not actually want UB here.
Consider an assert and returning json::Expr(nullptr)


================
Comment at: xpc/XPCJSONConversions.h:1
+//===--- XPCDispatcher.h - Main XPC service entry point ---------*- C++ -*-===//
+//
----------------
nit: header names wrong file.


================
Comment at: xpc/XPCJSONConversions.h:1
+//===--- XPCDispatcher.h - Main XPC service entry point ---------*- C++ -*-===//
+//
----------------
sammccall wrote:
> nit: header names wrong file.
consider just calling this `Conversion.h` or similar - it's already in the `xpc/` dir and, it seems like XPC conversions that **weren't** JSON-related would be likely to go here anyhow?


================
Comment at: xpc/XPCJSONConversions.h:19
+
+xpc_object_t jsonToXpc(const json::Expr& json);
+json::Expr xpcToJson(const xpc_object_t& json);
----------------
you could consider calling these `json::Expr toJSON(const xpc_object_t&)` and `bool fromJSON(const json::Expr&, xpc_object_t&)` to fit in with the json lib's convention - not sure if it actually buys you anything though.

(If so they should be in the global namespace so ADL works)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48560