[PATCH] D84525: [clangd] Add marshalling code for all request types

[Kadir Cetinkaya via Phabricator via cfe-commits]

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