[PATCH] D48560: [clangd] JSON <-> XPC conversions
jkorous added a comment. BTW Just for the record - I put the rebased & updated patch here: [clangd] XPC WIP https://reviews.llvm.org/D49548 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48560 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48560: [clangd] JSON <-> XPC conversions
jkorous marked 12 inline comments as done.
jkorous added inline comments.
Comment at: xpc/XPCJSONConversions.cpp:62
+ if (objType == XPC_TYPE_UINT64)
+return json::Expr(xpc_uint64_get_value(xpcObj));
+ if (objType == XPC_TYPE_STRING)
sammccall wrote:
> hmm, I think this shouldn't compile anymore, rather require you to explicitly
> do the narrowing conversion to int64 or double.
Explicitly narroving now. Thanks.
Comment at: xpc/XPCJSONConversions.cpp:69
+ xpcObj,
+ ^bool(size_t /* index */, xpc_object_t value) {
+result.emplace_back(xpcToJson(value));
jkorous wrote:
> 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.
Allright, by trying to get rid of C block in dictionary conversion I remembered
that there's unfortunately no sane reason how to iterate XPC dictionary without
using xpc_dictionary_apply() which uses C block for functor parameter.
https://developer.apple.com/documentation/xpc/1505404-xpc_dictionary_apply?language=objc
https://developer.apple.com/documentation/xpc/xpc_dictionary_applier_t?language=objc
Anyway, even had we succeeded in removing C blocks from conversion they still
would be necessary for dispatch as xpc_connection_set_event_handler() is
another part of XPC API that uses it.
I guess that there's no point in removing the xpc_array_apply() then. Is that
ok with you?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48560
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48560: [clangd] JSON <-> XPC conversions
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 getAsNumber() const {
if (LLVM_LIKELY(Type == T_Double))
return as();
if (LLVM_LIKELY(Type == T_Integer))
return as();
return llvm::None;
}
// Succeeds if the Value is a Number, and exactly representable as int64_t.
llvm::Optional getAsInteger() const {
if (LLVM_LIKELY(Type == T_Integer))
return as();
if (LLVM_LIKELY(Type == T_Double)) {
double D = as();
if (LLVM_LIKELY(std::modf(D, ) == 0.0 &&
D >= double(std::numeric_limits::min()) &&
D <= double(std::numeric_limits::max(
return D;
}
return llvm::None;
}
```
while both
```enum ValueType```
and
```template T () 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)
[PATCH] D48560: [clangd] JSON <-> XPC conversions
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48560: [clangd] JSON <-> XPC conversions
jkorous created this revision.
jkorous added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, MaskRay, ioeric, ilya-biryukov, mgorny.
This is a self-contained pair of utility functions for the XPC transport layer.
It's not dependent on but following the refactoring patch:
https://reviews.llvm.org/D48559
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48560
Files:
clangd/xpc/CMakeLists.txt
clangd/xpc/XPCJSONConversionTests.cpp
xpc/XPCJSONConversions.cpp
xpc/XPCJSONConversions.h
Index: clangd/xpc/XPCJSONConversionTests.cpp
===
--- /dev/null
+++ clangd/xpc/XPCJSONConversionTests.cpp
@@ -0,0 +1,452 @@
+//===-- XPCJSONConversion.cpp *- C++ -*---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "xpc/XPCJSONConversions.h"
+#include "gtest/gtest.h"
+
+#include
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(JsonXpcConversionTest, Null) {
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(nullptr)),
+ xpc_null_create()
+)
+ );
+ EXPECT_TRUE(
+json::Expr(nullptr) == xpcToJson(xpc_null_create())
+ );
+}
+
+TEST(JsonXpcConversionTest, Bool) {
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(true)),
+ xpc_bool_create(true)
+)
+ );
+ EXPECT_TRUE(
+json::Expr(false) == xpcToJson(xpc_bool_create(false))
+ );
+}
+
+TEST(JsonXpcConversionTest, Number) {
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(3.14)),
+ xpc_double_create(3.14)
+)
+ );
+ EXPECT_TRUE(
+json::Expr(3.14) == xpcToJson(xpc_double_create(3.14))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(42)),
+ xpc_double_create(42)
+)
+ );
+ EXPECT_TRUE(
+json::Expr(42) == xpcToJson(xpc_double_create(42))
+ );
+ EXPECT_TRUE(
+json::Expr(42) == xpcToJson(xpc_int64_create(42))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(-100)),
+ xpc_double_create(-100)
+)
+ );
+ EXPECT_TRUE(
+json::Expr(-100) == xpcToJson(xpc_double_create(-100))
+ );
+
+ unsigned long long bigPositiveValue = std::numeric_limits::max();
+ ++bigPositiveValue;
+ unsigned long long bigNegativeValue = std::numeric_limits::min();
+ --bigNegativeValue;
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(bigPositiveValue)),
+ xpc_double_create(bigPositiveValue)
+)
+ );
+ EXPECT_TRUE(
+json::Expr(bigPositiveValue) == xpcToJson(xpc_double_create(bigPositiveValue))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(bigNegativeValue)),
+ xpc_double_create(bigNegativeValue)
+)
+ );
+ EXPECT_TRUE(
+json::Expr(bigNegativeValue) == xpcToJson(xpc_double_create(bigNegativeValue))
+ );
+}
+
+TEST(JsonXpcConversionTest, String) {
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr("foo")),
+ xpc_string_create("foo")
+)
+ );
+ EXPECT_TRUE(
+json::Expr("foo") == xpcToJson(xpc_string_create("foo"))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr("")),
+ xpc_string_create("")
+)
+ );
+ EXPECT_TRUE(
+json::Expr("") == xpcToJson(xpc_string_create(""))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr("123")),
+ xpc_string_create("123")
+)
+ );
+ EXPECT_TRUE(
+json::Expr("123") == xpcToJson(xpc_string_create("123"))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(" ")),
+ xpc_string_create(" ")
+)
+ );
+ EXPECT_TRUE(
+json::Expr(" ") == xpcToJson(xpc_string_create(" "))
+ );
+
+ // Testing two different "patterns" just in case.
+ std::string kBStringOfAs("A", 1024);
+ std::string kBStringOfBs("B", 1024);
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(kBStringOfAs.c_str())),
+ xpc_string_create(kBStringOfAs.c_str())
+)
+ );
+ EXPECT_TRUE(
+json::Expr(kBStringOfAs.c_str()) == xpcToJson(xpc_string_create(kBStringOfAs.c_str()))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Expr(kBStringOfBs.c_str())),
+ xpc_string_create(kBStringOfBs.c_str())
+)
+ );
+ EXPECT_TRUE(
+json::Expr(kBStringOfBs.c_str()) == xpcToJson(xpc_string_create(kBStringOfBs.c_str()))
+ );
+}
+
+TEST(JsonXpcConversionTest, Array) {
+ json::Expr JsonArray{true, "foo", nullptr, 42};
+
+ xpc_object_t XpcArray = []() {
+std::vector XpcArrayElements;
+XpcArrayElements.emplace_back(xpc_bool_create(true));
+XpcArrayElements.emplace_back(xpc_string_create("foo"));
+XpcArrayElements.emplace_back(xpc_null_create());
+XpcArrayElements.emplace_back(xpc_double_create(42));
+
+return xpc_array_create(XpcArrayElements.data(), XpcArrayElements.size());
+ }();
