kadircet updated this revision to Diff 301280.
kadircet marked 9 inline comments as done.
kadircet added a comment.
- Drop PathSpec, deduce it from emptyness of FragmentDirectory.
- Make tests more homogenous.
- Add path cannonicalization to compile stage.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90270/new/
https://reviews.llvm.org/D90270
Files:
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigProvider.cpp
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -10,6 +10,7 @@
#include "ConfigProvider.h"
#include "ConfigTesting.h"
#include "TestFS.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/SourceMgr.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -187,6 +188,36 @@
EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
}
+TEST(ProviderTest, SourceInfo) {
+ MockFS FS;
+
+ FS.Files["baz/foo.yaml"] = R"yaml(
+If:
+ PathMatch: .*
+ PathExclude: bar.h
+CompileFlags:
+ Add: bar
+)yaml";
+ const auto BarPath = testPath("baz/bar.h", llvm::sys::path::Style::posix);
+ CapturedDiags Diags;
+ Params Bar;
+ Bar.Path = BarPath;
+
+ // This should be an absolute match/exclude hence baz/bar.h should not be
+ // excluded.
+ auto P = Provider::fromYAMLFile(testPath("baz/foo.yaml"), FS);
+ auto Cfg = P->getConfig(Bar, Diags.callback());
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar"));
+ Diags.Diagnostics.clear();
+
+ // This should be a relative match/exclude hence baz/bar.h should be excluded.
+ P = Provider::fromAncestorRelativeYAMLFiles("foo.yaml", FS);
+ Cfg = P->getConfig(Bar, Diags.callback());
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
+ Diags.Diagnostics.clear();
+}
} // namespace
} // namespace config
} // namespace clangd
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -9,8 +9,12 @@
#include "Config.h"
#include "ConfigFragment.h"
#include "ConfigTesting.h"
+#include "TestFS.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include <string>
namespace clang {
namespace clangd {
@@ -116,6 +120,62 @@
"Invalid Background value 'Foo'. Valid values are Build, Skip.")));
}
+TEST_F(ConfigCompileTests, PathSpecMatch) {
+ auto BarPath = llvm::sys::path::convert_to_slash(testPath("foo/bar.h"));
+ Parm.Path = BarPath;
+
+ struct {
+ std::string Directory;
+ std::string PathSpec;
+ bool ShouldMatch;
+ } Cases[] = {
+ {
+ // Absolute path matches.
+ "",
+ llvm::sys::path::convert_to_slash(testPath("foo/bar.h")),
+ true,
+ },
+ {
+ // Absolute path fails.
+ "",
+ llvm::sys::path::convert_to_slash(testPath("bar/bar.h")),
+ false,
+ },
+ {
+ // Relative should fail to match as /foo/bar.h doesn't reside under
+ // /baz/.
+ testPath("baz"),
+ "bar\\.h",
+ false,
+ },
+ {
+ // Relative should pass with /foo as directory.
+ testPath("foo"),
+ "bar\\.h",
+ true,
+ },
+ };
+
+ // PathMatch
+ for (const auto &Case : Cases) {
+ Frag = {};
+ Frag.If.PathMatch.emplace_back(Case.PathSpec);
+ Frag.Source.Directory = Case.Directory;
+ EXPECT_EQ(compileAndApply(), Case.ShouldMatch);
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ }
+
+ // PathEclude
+ for (const auto &Case : Cases) {
+ SCOPED_TRACE(Case.Directory);
+ SCOPED_TRACE(Case.PathSpec);
+ Frag = {};
+ Frag.If.PathExclude.emplace_back(Case.PathSpec);
+ Frag.Source.Directory = Case.Directory;
+ EXPECT_NE(compileAndApply(), Case.ShouldMatch);
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ }
+}
} // namespace
} // namespace config
} // namespace clangd
Index: clang-tools-extra/clangd/ConfigProvider.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigProvider.cpp
+++ clang-tools-extra/clangd/ConfigProvider.cpp
@@ -13,9 +13,11 @@
#include "support/Trace.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Path.h"
#include <chrono>
#include <mutex>
+#include <string>
namespace clang {
namespace clangd {
@@ -51,14 +53,18 @@
// Finally parse and compile the actual fragments.
for (auto &Fragment :
- Fragment::parseYAML(Buf->get()->getBuffer(), Path, DC))
+ Fragment::parseYAML(Buf->get()->getBuffer(), Path, DC)) {
+ Fragment.Source.Directory = Dir;
CachedValue.push_back(std::move(Fragment).compile(DC));
+ }
}
public:
// Must be set before the cache is used. Not a constructor param to allow
// computing ancestor-relative paths to be deferred.
std::string Path;
+ // Directory associated with this fragment.
+ std::string Dir;
// Retrieves up-to-date config fragments from disk.
// A cached result may be reused if the mtime and size are unchanged.
@@ -170,6 +176,7 @@
llvm::SmallString<256> ConfigPath = Ancestor;
path::append(ConfigPath, RelPath);
R.first->second.Path = ConfigPath.str().str();
+ R.first->second.Dir = Ancestor.str();
}
Caches.push_back(&R.first->second);
}
@@ -177,8 +184,9 @@
// Finally query each individual file.
// This will take a (per-file) lock for each file that actually exists.
std::vector<CompiledFragment> Result;
- for (FileConfigCache *Cache : Caches)
+ for (FileConfigCache *Cache : Caches) {
Cache->read(FS, DC, P.FreshTime, Result);
+ }
return Result;
};
Index: clang-tools-extra/clangd/ConfigFragment.h
===================================================================
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -91,6 +91,9 @@
/// The start of the original source for this fragment.
/// Only valid if SourceManager is set.
llvm::SMLoc Location;
+ /// Absolute path to directory the fragment is associated with. Relative
+ /// paths mentioned in the fragment are resolved against this.
+ std::string Directory;
};
SourceInfo Source;
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -31,9 +31,11 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Regex.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/SourceMgr.h"
+#include <string>
namespace clang {
namespace clangd {
@@ -70,6 +72,9 @@
CompiledFragmentImpl &Out;
DiagnosticCallback Diagnostic;
llvm::SourceMgr *SourceMgr;
+ // Directory containing the fragment. Absolute path with forward slashes, with
+ // a trailing slash or empty for user-config fragments.
+ std::string FragmentDirectory;
llvm::Optional<llvm::Regex> compileRegex(const Located<std::string> &Text) {
std::string Anchored = "^(" + *Text + ")$";
@@ -129,6 +134,11 @@
}
void compile(Fragment &&F) {
+ if (!F.Source.Directory.empty()) {
+ FragmentDirectory = llvm::sys::path::convert_to_slash(F.Source.Directory);
+ if (FragmentDirectory.back() != '/')
+ FragmentDirectory += '/';
+ }
compile(std::move(F.If));
compile(std::move(F.CompileFlags));
compile(std::move(F.Index));
@@ -145,11 +155,19 @@
}
if (!PathMatch->empty()) {
Out.Conditions.push_back(
- [PathMatch(std::move(PathMatch))](const Params &P) {
+ [PathMatch(std::move(PathMatch)),
+ FragmentDir(FragmentDirectory)](const Params &P) {
if (P.Path.empty())
return false;
+ llvm::StringRef Path = P.Path;
+ // Ignore the file if it is not nested under Fragment and strip the
+ // prefix.
+ if (!Path.consume_front(FragmentDir))
+ return false;
+ if (!FragmentDir.empty())
+ Path.consume_front("/");
return llvm::any_of(*PathMatch, [&](const llvm::Regex &RE) {
- return RE.match(P.Path);
+ return RE.match(Path);
});
});
}
@@ -161,11 +179,19 @@
}
if (!PathExclude->empty()) {
Out.Conditions.push_back(
- [PathExclude(std::move(PathExclude))](const Params &P) {
+ [PathExclude(std::move(PathExclude)),
+ FragmentDir(FragmentDirectory)](const Params &P) {
if (P.Path.empty())
return false;
+ llvm::StringRef Path = P.Path;
+ // Ignore the file if it is not nested under Fragment and strip the
+ // prefix.
+ if (!Path.consume_front(FragmentDir))
+ return true;
+ if (!FragmentDir.empty())
+ Path.consume_front("/");
return llvm::none_of(*PathExclude, [&](const llvm::Regex &RE) {
- return RE.match(P.Path);
+ return RE.match(Path);
});
});
}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
