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