[PATCH] D63961: [clangd][xpc] pass the LSP value using data instead of string

[Jan Korous via Phabricator via cfe-commits]

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