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