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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:53
 
-clangd::FuzzyFindRequest
-Marshaller::fromProtobuf(const FuzzyFindRequest *Request) {
+namespace {
+template <typename MessageT>
----------------
nit: move anon namespace to the top


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:55
+template <typename MessageT>
+llvm::Optional<llvm::DenseSet<SymbolID>> getIDs(MessageT *Message) {
+  llvm::DenseSet<SymbolID> Result;
----------------
I would make this return an expected instead of an optional and print the error in the callers, instead of logging twice.

(I also think a similar problem is present with the existing `fromProtobuf` APIs too, they return None in case of failure while logging the error, and then callers again log the error. I am not sure if we plan to implement some error handling strategies but even if we did, it feels like Marshalling is the lowest layer that wouldn't know any other strategy to handle such errors, so it seems like always returning an error from the marshalling and letting the caller handle the error sounds like a safe bet. But no action needed in this patch just stating some ideas in case you find them useful :D)


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:292
   auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
-  EXPECT_THAT(Deserialized.ProximityPaths,
+  EXPECT_TRUE(Deserialized);
+  EXPECT_THAT(Deserialized->ProximityPaths,
----------------
`ASSERT_TRUE` to ensure we don't try to dereference in case of None.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84525