[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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84939