[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:111
+  Req.IDs = std::move(*IDs);
+  Req.Filter = static_cast<RefKind>(Message->filter());
+  if (Message->limit())
----------------
can you also add tests for these? (well actually it looks like there are no tests for LookupRequests too)


================
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;
----------------
kbobyrev wrote:
> 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?
> 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.

i believe rest of the file doesn't handle such low level primitives, i.e. this one is specifically for extracting symbol ids, not for converting bigger proto messages into native structs (as done in the rest of the file)

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

i agree that it might be the case in other places, I haven't really looked at them carefully and they are also submitted (hence they were in parentheses in my previous comment). But I think the situation specifically in here is a little bit different.
There is a more clear struct for indicating missing fields in here, an empty set of results(and actually you are using that already), and using None to indicate an error. I believe using an `Error` (through an `Expected`) is a lot clear to indicate that.

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

As I mentioned, I believe this one is different than other places, and at least my taste/reasoning says that this should clearly be an Expected rather than an Optional. But it is not a big issue for me totally up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84525