jkorous added a comment.
I'd add a test with non-empty non-LSP dictionary to specifically test that
we're ignoring the content. I like const-correctness but that's up to you.
Otherwise LGTM.
================
Comment at: clang-tools-extra/clangd/unittests/xpc/ConversionTests.cpp:34
+TEST(JsonXpcConversionTest, IgnoreNonLSPDictionary) {
+ xpc_object_t EmptyDict = xpc_dictionary_create(nullptr, nullptr, 0);
+ json::Value Val = xpcToJson(EmptyDict);
----------------
We should also test something like
```
const char* key = "NotAnLSP";
const char* value = "Foo";
xpc_object_t EmptyDict = xpc_dictionary_create(&key, &value, 1);
```
================
Comment at: clang-tools-extra/clangd/xpc/Conversion.cpp:22
const char *const Key = "LSP";
- xpc_object_t PayloadObj = xpc_string_create(llvm::to_string(JSON).c_str());
+ std::string Str = llvm::to_string(JSON);
+ xpc_object_t PayloadObj = xpc_data_create(Str.data(), Str.size());
----------------
Nit - `const std::string`?
================
Comment at: clang-tools-extra/clangd/xpc/Conversion.cpp:29
if (xpc_get_type(XPCObject) == XPC_TYPE_DICTIONARY) {
- const char *const LSP = xpc_dictionary_get_string(XPCObject, "LSP");
- auto Json = json::parse(llvm::StringRef(LSP));
+ size_t Length;
+ const char *LSP =
----------------
Nit -`const size_t` and `const char * const`?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63961/new/
https://reviews.llvm.org/D63961
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
