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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
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;
----------------
kadircet wrote:
> 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)
Good point: we thought about error handling and discussed it with Sam on multiple occasions, we tested several strategies and I think converged to this. I don't think it's optimal, you are right, but this code is consistent with the rest of the file.

The problem here is that in some cases these should actually be `Optional`s (some fields may or may not be missing) and in some cases it makes sense to have `Expected`s, which would leave a weird mixture of these things. But maybe carefully thinking about those cases would simplify the code.

How do you feel about leaving this as is and maybe carefully thinking about `Optional`s to `Expected` in the next patches?


================
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,
----------------
kadircet wrote:
> `ASSERT_TRUE` to ensure we don't try to dereference in case of None.
Good point, I should do `ASSERT_TRUE` in most cases throughout the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84525