sammccall added a comment.
Thanks for doing this!
Main points:
- I think we should have one high-level concept of "the optional associated
directory" rather than two low-level ones of "the location of the config file"
and "should we use the location to form relative paths"
- We need to be careful about windows vs posix paths
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:277
- FragmentCompiler{*Result, D, Source.Manager.get()}.compile(std::move(*this));
+ FragmentCompiler{*Result, D, Source.Manager.get(), Source}.compile(
+ std::move(*this));
----------------
passing a reference to part of `this` and then separately `this` by rvalue when
the reference is live feels... fragile.
Maybe have compile() assign to a member stringref instead? Or even a string and
do the path normalization at that point...
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:94
llvm::SMLoc Location;
+ /// Absolute path to directory containing the fragment. Doesn't contain the
+ /// trailing slash.
----------------
directory the fragment is associated with? At this level, I don't think we
should care whether it's actually a file that lives there, and I don't see why
the global config would want to set this.
We should also say what this is for: "Relative paths mentioned the fragment are
resolved against this directory."
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:95
+ /// Absolute path to directory containing the fragment. Doesn't contain the
+ /// trailing slash.
+ std::string FragmentDirectory;
----------------
Paths in a fragment are conventionally Posix paths, so this dir should be one
too before we can use string prefixes. It's also more convenient if it has a
trailing slash (current code has a bug where /foo matches /foo2).
This could be handled by the creator of SourceInfo, or we could accept native
paths here (with/without slashes) and normalize at the start of compilation.
Latter is probably slightly less error-prone.
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:96
+ /// trailing slash.
+ std::string FragmentDirectory;
+ /// Describes how paths specified in this fragment should be treated.
----------------
Not sure "Fragment" adds anything here - Directory is a little vaguer than
ideal but FragmentDirectory just seems... longer.
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:98
+ /// Describes how paths specified in this fragment should be treated.
+ enum PathSpecification {
+ Absolute,
----------------
Why is this separate from FragmentDirectory?
(i.e. when would is it not `FragmentDirectory.empty() ? Absolute : Relative`)
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:36
+ void fillCacheFromDisk(llvm::vfs::FileSystem &FS, DiagnosticCallback DC,
+ Fragment::SourceInfo::PathSpecification PathSpec) {
CachedValue.clear();
----------------
the associated directory is invariant, like the path - consider making it a
member set at construction time instead of plumbing it through all the functions
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:124
std::vector<CompiledFragment> Result;
- Cache.read(FS, DC, P.FreshTime, Result);
+ Cache.read(FS, DC, P.FreshTime, Fragment::SourceInfo::Absolute, Result);
return Result;
----------------
this seems like a surprising place to encode this choice (why is fromYAMLFile
coupled to the idea that it's a global config file not associated with any dir?)
================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:122
+ Fragment Frag;
+ Frag.If.PathMatch.emplace_back(".*");
+ Frag.Source.FragmentDirectory = "/baz";
----------------
can we use a slightly less degenerate example to verify that we're applying the
regex to the correct string?
================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:123
+ Frag.If.PathMatch.emplace_back(".*");
+ Frag.Source.FragmentDirectory = "/baz";
+ return Frag;
----------------
please use testPath() to generate fake paths.
I think this would have caught path-handling bugs on windows.
================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:146
+
+TEST_F(ConfigCompileTests, PathSpecExclude) {
+ const Fragment BaseFrag = [] {
----------------
can we fold this into the test above? it seems pretty duplicative.
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
