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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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