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