kadircet added a comment.
mostly LG. sorry for lots of nits, only two important bits are changing the {1}
to {0} in logs, wrapping symbolid generations in `llvm::cantFail` and making
sure `Deserialized` is exactly the same thing as `Request` in tests. feel free
to ignore the rest (I should've marked them with `nit` hopefully, but might've
missed some)
================
Comment at:
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:104
+ auto IDs = getIDs(Message);
+ if (!IDs) {
+ return IDs.takeError();
----------------
nit: braces
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:65
+ if (!Req) {
+ elog("Can not parse LookupRequest from protobuf: {1}", Req.takeError());
+ return grpc::Status::CANCELLED;
----------------
printing indices are zero-based so
s/{1}/{0}/g
(same for others)
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:277
+ clangd::LookupRequest Request;
+ auto ID = SymbolID::fromStr("057557CEBF6E6B2D");
+ ASSERT_TRUE(bool(ID));
----------------
you can wrap these in `llvm::cantFail` as the symbolid generation bit is
constant, and isn't really something we are testing here
nit: also I would suggest a less random string to indicate that these are
arbitrary like 0000...001 (the length still needs to match tho)
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:287
+ auto Serialized = ProtobufMarshaller.toProtobuf(Request);
+ EXPECT_EQ(Serialized.ids_size(), 2);
+ auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
----------------
nit: instead of `2` use `Request.IDs.size()`
Moreover, `SymbolID` has an `opreator==` so in theory you could do an
`EXPECT_THAT(Serialized.ids(), ElementsAreArray(Request.IDs))` trick to make
this check even nicer. But I am not sure about how nicely repeated proto fields
plays with gtest version in llvm. So if it doesn't work nvm. (same for the
other places with this pattern)
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:290
+ ASSERT_TRUE(bool(Deserialized));
+ EXPECT_EQ(Deserialized->IDs.size(), 2U);
+
----------------
you should definitely be able to make sure `IDs` are exactly the same with
`Request.IDs`
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:292
+
+ Serialized.add_ids("Invalid Symbol ID");
+ Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
----------------
nit: I would put this one into a separate test
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:328
+ auto Serialized = ProtobufMarshaller.toProtobuf(Request);
+ EXPECT_EQ(Serialized.ids_size(), 2);
+ EXPECT_EQ(Serialized.limit(), 9000U);
----------------
again I would suggest comparing to Requests.IDs.size(), and other relevant
fields below instead of repeating some constants (which would be tedious to
change in future)
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:332
+ ASSERT_TRUE(bool(Deserialized));
+ EXPECT_EQ(Deserialized->IDs.size(), 2U);
+ ASSERT_TRUE(Deserialized->Limit);
----------------
again I would make sure `IDs` are same element-wise in here.
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:337
+
+ Serialized.add_ids("Invalid Symbol ID");
+ Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
----------------
nit: again this could be a separate test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84525/new/
https://reviews.llvm.org/D84525
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
