kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
================
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.",
----------------
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.
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:169
+ ASSERT_TRUE(bool(Serialized));
+ EXPECT_TRUE(bool(ProtobufMarshaller.fromProtobuf(*Serialized)));
----------------
i would ASSERT_TRUE here, as you'll end up crashing when `fromProtoBuf` returns
an error (because you are not consuming the error and it is destroyed after
completion of the statement).
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:418
+ auto URIString = ProtobufMarshaller.relativePathToURI("lib/File.cpp");
+ EXPECT_TRUE(bool(URIString));
// RelativePath can not be absolute.
----------------
again should be ASSERT_TRUE for similar concerns stated above.
================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:436
+ testPathURI("remote/project/lib/File.cpp", Strings));
+ EXPECT_TRUE(bool(RelativePath));
// RemoteIndexRoot has to be be a prefix of the file path.
----------------
again should be ASSERT_TRUE for similar concerns stated above.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84939/new/
https://reviews.llvm.org/D84939
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
