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

[Jan Korous via Phabricator via cfe-commits]

jkorous marked an inline comment as done.
jkorous added a comment.

Hi Sam, no worries! Thanks for making time to take a look! I am going to rebase all my patches on top of JSON library moved to llvm/Support and process your comments.



================
Comment at: clangd/xpc/CMakeLists.txt:1
+set(LLVM_LINK_COMPONENTS
+  support
----------------
sammccall wrote:
> 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.
That's a good idea, thanks.


================
Comment at: clangd/xpc/CMakeLists.txt:15
+
+# FIXME: Factor out json::Expr from clangDaemon
+target_link_libraries(ClangdXpcTests
----------------
sammccall wrote:
> this is done :-)
Yes, I know - thanks for the good work with lifting the JSON library! I am going to rebase my patches.


================
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());
----------------
sammccall wrote:
> 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)
I was initially thinking along the same line but got stuck on the fact that JSON standard (modelled by llvm::json::Value) doesn't distinguish numeric types (it only has Number).

Relevant part of llvm::json::Value interface is:
```
llvm::Optional<double> getAsNumber() const {
    if (LLVM_LIKELY(Type == T_Double))
      return as<double>();
    if (LLVM_LIKELY(Type == T_Integer))
      return as<int64_t>();
    return llvm::None;
  }
  // Succeeds if the Value is a Number, and exactly representable as int64_t.
  llvm::Optional<int64_t> getAsInteger() const {
    if (LLVM_LIKELY(Type == T_Integer))
      return as<int64_t>();
    if (LLVM_LIKELY(Type == T_Double)) {
      double D = as<double>();
      if (LLVM_LIKELY(std::modf(D, &D) == 0.0 &&
                      D >= double(std::numeric_limits<int64_t>::min()) &&
                      D <= double(std::numeric_limits<int64_t>::max())))
        return D;
    }
    return llvm::None;
  }
```
while both
```enum ValueType```
and
```template<typename T> T &as() const```
are private which makes sense since those are implementation details. 

Basically it seems to me there's no reliable way how to tell if Value is double or not (my example is 2.0) which means we couldn't preserve the "static" numeric type (it would be value dependent). I don't think changing the JSON library because of this would be a good idea. It's not a big deal for us and it's possible we'll skip JSON completely in XPC transport layer in the future.


================
Comment at: xpc/XPCJSONConversions.cpp:40
+      }
+      // Get pointers only now so there's no possible re-allocation of keys.
+      for(const auto& k : keys)
----------------
sammccall wrote:
> (you could also pre-size the vector, or use a deque)
Ok, I'll preallocate necessary space. I remember Bjarne Stroustrup saying he doesn't bother with reserve() anymore but it (most probably) won't do any harm here.

http://www.stroustrup.com/bs_faq2.html#slow-containers
"People sometimes worry about the cost of std::vector growing incrementally. I used to worry about that and used reserve() to optimize the growth. After measuring my code and repeatedly having trouble finding the performance benefits of reserve() in real programs, I stopped using it except where it is needed to avoid iterator invalidation (a rare case in my code). Again: measure before you optimize."


================
Comment at: xpc/XPCJSONConversions.cpp:69
+      xpcObj,
+      ^bool(size_t /* index */, xpc_object_t value) {
+        result.emplace_back(xpcToJson(value));
----------------
sammccall wrote:
> 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)
Oh, sorry. These are actually C blocks - a nonstandard C extension.
https://en.wikipedia.org/wiki/Blocks_(C_language_extension)
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Blocks/Articles/00_Introduction.html

There was no performance concern on my side - just liked this approach better. Didn't realize how confusing it might be, will rewrite this.


================
Comment at: xpc/XPCJSONConversions.cpp:87
+  }
+  llvm_unreachable("unsupported JSON data type in xpcToJson() conversion");
+}
----------------
sammccall wrote:
> 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)
That's a good point. Thanks!


================
Comment at: xpc/XPCJSONConversions.h:1
+//===--- XPCDispatcher.h - Main XPC service entry point ---------*- C++ -*-===//
+//
----------------
sammccall wrote:
> 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?
Good catch, thanks!
All right, I don't have any strong opinion about the filename so might as well rename it.


================
Comment at: xpc/XPCJSONConversions.h:19
+
+xpc_object_t jsonToXpc(const json::Expr& json);
+json::Expr xpcToJson(const xpc_object_t& json);
----------------
sammccall wrote:
> 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)
I don't have any strong opinion about this - just that I would slightly prefer consistency. I will process other comments first and decide in the meantime.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48560