[PATCH] D84894: [clangd] Implement Relations request for remote index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks mostly nits, only annoying bit is usage of optionals and multi-logging but letting them be for now :D.

also looks like there are some irrelevant changes, e.g. auto's in lambdas or formatting in protos, feel free to land an NFC change(without review) for those before landing this.



================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:382
+
+  auto Serialized = ProtobufMarshaller.toProtobuf(Request);
+  // Not a valid SymbolID.
----------------
nit: why not just do:
```
RelationsRequest Serialized;
Serialized.add_subjects("ZZZZZZZZZZZZZZZZ");
// check for failure during deserialization
```


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:391
+TEST(RemoteMarshallingTest, RelationsSerializion) {
+  clangd::Symbol Sym;
+  SymbolID ID = llvm::cantFail(SymbolID::fromStr("0000000000000001"));
----------------
nit: move it near `Sym.ID` assignemnt.


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:395
+
+  Sym.ID = llvm::cantFail(SymbolID::fromStr("0000000000000002"));
+
----------------
nit: I would move all of the following population logic into a helper and share between here and symbol serialization tests.


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:420
+  ASSERT_TRUE(Deserialized);
+  EXPECT_EQ(ID, Deserialized->first);
+  Sym.CanonicalDeclaration.FileURI = testPathURI(
----------------
nit: swap parameters `ID` is expected, `Deserialized->first` is actual.


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:423
+      "local/llvm-project/llvm/clang-tools-extra/clangd/Protocol.h", Strings);
+  EXPECT_EQ(toYAML(Sym), toYAML(Deserialized->second));
+
----------------
i think this is already tested in symbol deserialization test above, feel free to leave it out.


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:425
+
+  *Serialized->mutable_subject_id() = "Not A Symbol ID";
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(*Serialized));
----------------
again this should be part of symbol serialization tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84894/new/

https://reviews.llvm.org/D84894