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