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