[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:313
                              RelativePath, llvm::sys::path::Style::posix));
-  if (RelativePath.empty()) {
-    return llvm::None;
-  }
-  if (llvm::sys::path::is_absolute(RelativePath)) {
-    return llvm::None;
-  }
+  if (RelativePath.empty())
+    return make_error<StringError>("Empty relative path.",
----------------
kadircet wrote:
> kbobyrev wrote:
> > kadircet wrote:
> > > is `RelativePath` user provided? can't we just assert on non-emptiness and non-absoluteness and make this function return a std::string instead?
> > Could you elaborate on what you mean by user provided? Relative path is something that comes from the remote index. Ideally, this is non-empty and relative but I think the safer option is to maybe check for errors? Actually, I'm not sure about this but it's not user provided as in "given as a flag passed to CLI invocation by the end user".
> > 
> > I think the point here is making errors recoverable and not shutting down the client, otherwise everything could be an assertion? Same in other places.
> > 
> > WDYT?
> > Could you elaborate on what you mean by user provided? Relative path is something that comes from the remote index. 
> 
> Yes I was asking whether this comes from outside and ideally not processed at all before arriving at this logic.
> 
> > Ideally, this is non-empty and relative but I think the safer option is to maybe check for errors? Actually, I'm not sure about this but it's not user provided as in "given as a flag passed to CLI invocation by the end user".
> 
> Right, the safer is always to check for errors, but if we've already processed the RelativePath before it reaches this code path then there wouldn't be much value in spending more runtime checks (and also it would be callers responsibility generate the errors in such cases). I was asking this because I wasn't aware of the whole structure, but looks like this is literally the entry point (i.e. the first logic that verifies a user input) for path to uri conversions. So no action needed here.
Yes, I agree that spending runtime on this is unfortunate but I guess it's the cost we'd have to pay :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84939