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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
