[PATCH] D90270: [clangd] Handle absolute/relative path specifications in Config

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Still LG but a few more nits



================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:46
 
+// Returns an empty stringref if Path is not under FragmentDir.
+llvm::StringRef configRelative(llvm::StringRef Path,
----------------
you should document+verify what this does if FragmentDir is empty, since we are triggering that case


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:170
+          [PathMatch(std::move(PathMatch)),
+           FragmentDir(FragmentDirectory)](const Params &P) {
             if (P.Path.empty())
----------------
nit: why the rename rather than just capturing?


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:51
+    return llvm::StringRef();
+  return Path;
+}
----------------
kadircet wrote:
> sammccall wrote:
> > not that if Path == FragmentDir you're going to return "" which means "not under".
> > I don't think this can happen yet, but it's a trap for the future...
> > 
> > You might want the last line to be "return FragmentDir.empty() ? "." : Path".
> i think you meant `Path.empty() ? "." : Path`, right?
oops, no I think I meant `(Path.empty() && !FragmentDir.empty() ? "." : Path`.
You don't want configRelative("","") to be ".".

So either that or handle the FragmentDir-is-empty case upfront.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90270