[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
ymandel added inline comments.
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:286
+
+ virtual llvm::Optional TraversalKind() const
{
+return {};
aaron.ballman wrote:
> `traversalKind()`
Stephen -- What was the resolution on this comment? I came across this when
writing my recent fixes and it stood out for going against the style guide and
the surrounding style. Is this intentional or an oversight?
Thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire added a comment. I tried using DenseMap, but couldn't make it compile. I stuck with std::map for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61837/new/ https://reviews.llvm.org/D61837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0a717d5b5d31: Make it possible control matcher traversal
kind with ASTContext (authored by stephenkelly).
Changed prior to commit:
https://reviews.llvm.org/D61837?vs=230678&id=232661#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
Files:
clang/include/clang/AST/ASTContext.h
clang/include/clang/AST/ASTNodeTraverser.h
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/include/clang/ASTMatchers/ASTMatchersInternal.h
clang/lib/AST/ASTContext.cpp
clang/lib/ASTMatchers/ASTMatchFinder.cpp
clang/lib/ASTMatchers/ASTMatchersInternal.cpp
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1595,6 +1595,91 @@
notMatches("class C {}; C a = C();", varDecl(has(cxxConstructExpr();
}
+TEST(Traversal, traverseMatcher) {
+
+ StringRef VarDeclCode = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+)cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ EXPECT_TRUE(
+ notMatches(VarDeclCode, traverse(ast_type_traits::TK_AsIs, Matcher)));
+ EXPECT_TRUE(
+ matches(VarDeclCode,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ Matcher)));
+}
+
+TEST(Traversal, traverseMatcherNesting) {
+
+ StringRef Code = R"cpp(
+float bar(int i)
+{
+ return i;
+}
+
+void foo()
+{
+ bar(bar(3.0));
+}
+)cpp";
+
+ EXPECT_TRUE(matches(
+ Code,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ callExpr(has(callExpr(traverse(
+ ast_type_traits::TK_AsIs,
+ callExpr(has(implicitCastExpr(has(floatLiteral(;
+}
+
+TEST(Traversal, traverseMatcherThroughImplicit) {
+ StringRef Code = R"cpp(
+struct S {
+ S(int x);
+};
+
+void constructImplicit() {
+ int a = 8;
+ S s(a);
+}
+ )cpp";
+
+ auto Matcher = traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ implicitCastExpr());
+
+ // Verfiy that it does not segfault
+ EXPECT_FALSE(matches(Code, Matcher));
+}
+
+TEST(Traversal, traverseMatcherThroughMemoization) {
+
+ StringRef Code = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+ )cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ // Matchers such as hasDescendant memoize their result regarding AST
+ // nodes. In the matcher below, the first use of hasDescendant(Matcher)
+ // fails, and the use of it inside the traverse() matcher should pass
+ // causing the overall matcher to be a true match.
+ // This test verifies that the first false result is not re-used, which
+ // would cause the overall matcher to be incorrectly false.
+
+ EXPECT_TRUE(matches(
+ Code, functionDecl(anyOf(
+hasDescendant(Matcher),
+traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ functionDecl(hasDescendant(Matcher)));
+}
+
TEST(IgnoringImpCasts, MatchesImpCasts) {
// This test checks that ignoringImpCasts matches when implicit casts are
// present and its inner matcher alone does not match.
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -189,6 +189,14 @@
llvm_unreachable("Invalid Op value.");
}
+DynTypedMatcher DynTypedMatcher::constructRestrictedWrapper(
+const DynTypedMatcher &InnerMatcher,
+ast_type_traits::ASTNodeKind RestrictKind) {
+ DynTypedMatcher Copy = InnerMatcher;
+ Copy.RestrictKind = RestrictKind;
+ return Copy;
+}
+
DynTypedMatcher DynTypedMatcher::trueMatcher(
ast_type_traits::ASTNodeKind NodeKind) {
return DynTypedMatcher(NodeKind, NodeKind, &*TrueMatcherInstance);
@@ -211,8 +219,13 @@
bool DynTypedMatcher::matches(const ast_type_traits::DynTypedNode &DynNode,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ TraversalKindScope RAII(Finder->getASTContext(),
+ Implementation->TraversalKind());
+
+ auto N = Finder->getASTContext().traverseIgnored(DynNode);
+
+ if (RestrictKind.isBaseOf(N.getNodeKind()) &&
+ Implementation->dynMatches(N, Finder, Builder)) {
return true;
}
// Delete all bindings when a matcher does not match.
@@ -225,8 +238,13 @@
bool DynTypedMatcher::matchesNoKindCheck(
const ast_type_traits::DynTypedNode &DynNode, AS
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM with a few minor nits.
Comment at: clang/include/clang/AST/ASTContext.h:3009
class ParentMap;
- std::unique_ptr Parents;
+ std::map> Parents;
Do we need to use `std::map` here, or could we use an `IndexedMap` or
`DenseMap`?
(`std::map` tends to perform poorly under some circumstances and the keys in
this case look like they should be small and dense, but if we can't use one of
the more specialized containers for some reason, it's fine to stick with
`std::map`.)
Comment at: clang/lib/AST/ASTContext.cpp:120
+ASTContext::traverseIgnored(const ast_type_traits::DynTypedNode &N) const {
+ if (auto *E = N.get()) {
+return ast_type_traits::DynTypedNode::create(*traverseIgnored(E));
Because `N` is const, does its `get<>` method return a `const` pointer? If so,
switch to `const auto *` for clarity.
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:222
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ TraversalKindScope raii(Finder->getASTContext(),
+ Implementation->TraversalKind());
`raii` -> `RAII` per naming conventions.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire updated this revision to Diff 230678.
steveire added a comment.
Rebase and update
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
Files:
clang/include/clang/AST/ASTContext.h
clang/include/clang/AST/ASTNodeTraverser.h
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/include/clang/ASTMatchers/ASTMatchersInternal.h
clang/lib/AST/ASTContext.cpp
clang/lib/ASTMatchers/ASTMatchFinder.cpp
clang/lib/ASTMatchers/ASTMatchersInternal.cpp
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1595,6 +1595,91 @@
notMatches("class C {}; C a = C();", varDecl(has(cxxConstructExpr();
}
+TEST(Traversal, traverseMatcher) {
+
+ StringRef VarDeclCode = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+)cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ EXPECT_TRUE(
+ notMatches(VarDeclCode, traverse(ast_type_traits::TK_AsIs, Matcher)));
+ EXPECT_TRUE(
+ matches(VarDeclCode,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ Matcher)));
+}
+
+TEST(Traversal, traverseMatcherNesting) {
+
+ StringRef Code = R"cpp(
+float bar(int i)
+{
+ return i;
+}
+
+void foo()
+{
+ bar(bar(3.0));
+}
+)cpp";
+
+ EXPECT_TRUE(matches(
+ Code,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ callExpr(has(callExpr(traverse(
+ ast_type_traits::TK_AsIs,
+ callExpr(has(implicitCastExpr(has(floatLiteral(;
+}
+
+TEST(Traversal, traverseMatcherThroughImplicit) {
+ StringRef Code = R"cpp(
+struct S {
+ S(int x);
+};
+
+void constructImplicit() {
+ int a = 8;
+ S s(a);
+}
+ )cpp";
+
+ auto Matcher = traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ implicitCastExpr());
+
+ // Verfiy that it does not segfault
+ EXPECT_FALSE(matches(Code, Matcher));
+}
+
+TEST(Traversal, traverseMatcherThroughMemoization) {
+
+ StringRef Code = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+ )cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ // Matchers such as hasDescendant memoize their result regarding AST
+ // nodes. In the matcher below, the first use of hasDescendant(Matcher)
+ // fails, and the use of it inside the traverse() matcher should pass
+ // causing the overall matcher to be a true match.
+ // This test verifies that the first false result is not re-used, which
+ // would cause the overall matcher to be incorrectly false.
+
+ EXPECT_TRUE(matches(
+ Code, functionDecl(anyOf(
+hasDescendant(Matcher),
+traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ functionDecl(hasDescendant(Matcher)));
+}
+
TEST(IgnoringImpCasts, MatchesImpCasts) {
// This test checks that ignoringImpCasts matches when implicit casts are
// present and its inner matcher alone does not match.
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -189,6 +189,14 @@
llvm_unreachable("Invalid Op value.");
}
+DynTypedMatcher DynTypedMatcher::constructRestrictedWrapper(
+const DynTypedMatcher &InnerMatcher,
+ast_type_traits::ASTNodeKind RestrictKind) {
+ DynTypedMatcher Copy = InnerMatcher;
+ Copy.RestrictKind = RestrictKind;
+ return Copy;
+}
+
DynTypedMatcher DynTypedMatcher::trueMatcher(
ast_type_traits::ASTNodeKind NodeKind) {
return DynTypedMatcher(NodeKind, NodeKind, &*TrueMatcherInstance);
@@ -211,8 +219,13 @@
bool DynTypedMatcher::matches(const ast_type_traits::DynTypedNode &DynNode,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ TraversalKindScope raii(Finder->getASTContext(),
+ Implementation->TraversalKind());
+
+ auto N = Finder->getASTContext().traverseIgnored(DynNode);
+
+ if (RestrictKind.isBaseOf(N.getNodeKind()) &&
+ Implementation->dynMatches(N, Finder, Builder)) {
return true;
}
// Delete all bindings when a matcher does not match.
@@ -225,8 +238,13 @@
bool DynTypedMatcher::matchesNoKindCheck(
const ast_type_traits::DynTypedNode &DynNode, ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
- if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+ T
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire marked an inline comment as done.
steveire added inline comments.
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:240
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
aaron.ballman wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > Add an assertion message?
> > Saying what? The original code doesn't have one. Let's avoid round trips in
> > review comments :).
> This isn't a "round trip"; it's not unreasonable to ask people to NFC improve
> the code they're touching (it's akin to saying "Because you figured out this
> complex piece of code does X, can you add a comment to it so others don't
> have to do that work next time.").
>
> As best I can tell, this assertion exists because this function is meant to
> mirror `matches()` without this base check in release mode. You've lost that
> mirroring with your refactoring, which looks suspicious. Is there a reason
> this function deviates from `matches()` now?
>
> As for the assertion message itself, how about "matched a node of an
> unexpected derived kind"?
Hmm, maybe that was a misunderstanding. Your note about an assertion message
was not clear, so I asked you what you suggest the message should be. No need
for offense :)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
aaron.ballman added inline comments.
Comment at: lib/AST/ASTContext.cpp:120
+ASTContext::TraverseIgnored(const ast_type_traits::DynTypedNode &N) {
+ if (auto E = N.get()) {
+return ast_type_traits::DynTypedNode::create(*TraverseIgnored(E));
aaron.ballman wrote:
> `auto *`
The formatting is wrong here -- be sure to run the patch through clang-format
before committing.
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:240
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
steveire wrote:
> aaron.ballman wrote:
> > Add an assertion message?
> Saying what? The original code doesn't have one. Let's avoid round trips in
> review comments :).
This isn't a "round trip"; it's not unreasonable to ask people to NFC improve
the code they're touching (it's akin to saying "Because you figured out this
complex piece of code does X, can you add a comment to it so others don't have
to do that work next time.").
As best I can tell, this assertion exists because this function is meant to
mirror `matches()` without this base check in release mode. You've lost that
mirroring with your refactoring, which looks suspicious. Is there a reason this
function deviates from `matches()` now?
As for the assertion message itself, how about "matched a node of an unexpected
derived kind"?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire marked an inline comment as done.
steveire added inline comments.
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:240
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
aaron.ballman wrote:
> Add an assertion message?
Saying what? The original code doesn't have one. Let's avoid round trips in
review comments :).
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
aaron.ballman added inline comments.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:718
+template
+internal::Matcher traverse(ast_type_traits::TraversalKind TK,
+ const internal::Matcher &InnerMatcher) {
steveire wrote:
> aaron.ballman wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > Is this an API we should be exposing to clang-query as well? Will those
> > > > users be able to use a string literal for the traversal kind, like they
> > > > already do for attribute kinds (etc)?
> > > Yes, I thought about that, but in a follow-up patch. First, I aim to
> > > extend the `TraversalKind` enum with `TK_IgnoreInvisble`.
> > This is new functionality, so why do you want to wait for a follow-up patch
> > (is it somehow more involved)? We typically add support for dynamic
> > matchers at the same time we add support for the static matchers because
> > otherwise the two get frustratingly out of sync.
> Perhaps I can add the support in DynamicASTMatchers. Can you give some
> direction on that? I don't recall where the attr strings are handled.
>
> A corresponding change to `clang-query` will be in another patch anyway
> because it's a different repo.
> Perhaps I can add the support in DynamicASTMatchers. Can you give some
> direction on that? I don't recall where the attr strings are handled.
I always wind up having to use svn blame to figure it out, so you're not alone
in your recollections. ;-) You add a new specialization of `ArgTypeTraits` to
Marshallers.h for the enumeration type. See around line 123 or so for how it's
done for `attr::Kind`.
> A corresponding change to clang-query will be in another patch anyway because
> it's a different repo.
Once you have the enumeration being marshaled, I believe all you have to do is
expose the new API via Registry.cpp like any other matcher, though I'd also
request tests be added to Dynamic/RegistryTest.cpp since this is adding a small
amount of new marshaling code.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire marked 2 inline comments as done.
steveire added inline comments.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:718
+template
+internal::Matcher traverse(ast_type_traits::TraversalKind TK,
+ const internal::Matcher &InnerMatcher) {
aaron.ballman wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > Is this an API we should be exposing to clang-query as well? Will those
> > > users be able to use a string literal for the traversal kind, like they
> > > already do for attribute kinds (etc)?
> > Yes, I thought about that, but in a follow-up patch. First, I aim to extend
> > the `TraversalKind` enum with `TK_IgnoreInvisble`.
> This is new functionality, so why do you want to wait for a follow-up patch
> (is it somehow more involved)? We typically add support for dynamic matchers
> at the same time we add support for the static matchers because otherwise the
> two get frustratingly out of sync.
Perhaps I can add the support in DynamicASTMatchers. Can you give some
direction on that? I don't recall where the attr strings are handled.
A corresponding change to `clang-query` will be in another patch anyway because
it's a different repo.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire updated this revision to Diff 199884.
steveire added a comment.
Update
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
Files:
include/clang/AST/ASTContext.h
include/clang/AST/ASTNodeTraverser.h
include/clang/ASTMatchers/ASTMatchers.h
include/clang/ASTMatchers/ASTMatchersInternal.h
lib/AST/ASTContext.cpp
lib/ASTMatchers/ASTMatchFinder.cpp
lib/ASTMatchers/ASTMatchersInternal.cpp
unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1510,6 +1510,72 @@
notMatches("class C {}; C a = C();", varDecl(has(cxxConstructExpr();
}
+TEST(Traversal, traverseMatcher) {
+
+ StringRef VarDeclCode = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+)cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ EXPECT_TRUE(
+ notMatches(VarDeclCode, traverse(ast_type_traits::TK_AsIs, Matcher)));
+ EXPECT_TRUE(
+ matches(VarDeclCode,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ Matcher)));
+}
+
+TEST(Traversal, traverseMatcherNesting) {
+
+ StringRef Code = R"cpp(
+float bar(int i)
+{
+ return i;
+}
+
+void foo()
+{
+ bar(bar(3.0));
+}
+)cpp";
+
+ EXPECT_TRUE(matches(
+ Code,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ callExpr(has(callExpr(traverse(
+ ast_type_traits::TK_AsIs,
+ callExpr(has(implicitCastExpr(has(floatLiteral(;
+}
+
+TEST(Traversal, traverseMatcherThroughMemoization) {
+
+ StringRef Code = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+ )cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ // Matchers such as hasDescendant memoize their result regarding AST
+ // nodes. In the matcher below, the first use of hasDescendant(Matcher)
+ // fails, and the use of it inside the traverse() matcher should pass
+ // causing the overall matcher to be a true match.
+ // This test verifies that the first false result is not re-used, which
+ // would cause the overall matcher to be incorrectly false.
+
+ EXPECT_TRUE(matches(
+ Code, functionDecl(anyOf(
+hasDescendant(Matcher),
+traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ functionDecl(hasDescendant(Matcher)));
+}
+
TEST(IgnoringImpCasts, MatchesImpCasts) {
// This test checks that ignoringImpCasts matches when implicit casts are
// present and its inner matcher alone does not match.
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -211,10 +211,19 @@
bool DynTypedMatcher::matches(const ast_type_traits::DynTypedNode &DynNode,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto PreviousTraversalKind = Finder->getASTContext().GetTraversalKind();
+ auto OptTK = Implementation->TraversalKind();
+ if (OptTK)
+Finder->getASTContext().SetTraversalKind(*OptTK);
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ if (RestrictKind.isBaseOf(NodeKind) &&
+ Implementation->dynMatches(N, Finder, Builder)) {
+Finder->getASTContext().SetTraversalKind(PreviousTraversalKind);
return true;
}
+ Finder->getASTContext().SetTraversalKind(PreviousTraversalKind);
// Delete all bindings when a matcher does not match.
// This prevents unexpected exposure of bound nodes in unmatches
// branches of the match tree.
@@ -225,8 +234,11 @@
bool DynTypedMatcher::matchesNoKindCheck(
const ast_type_traits::DynTypedNode &DynNode, ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
- if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
return true;
}
// Delete all bindings when a matcher does not match.
Index: lib/ASTMatchers/ASTMatchFinder.cpp
===
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -59,10 +59,12 @@
DynTypedMatcher::MatcherIDType MatcherID;
ast_type_traits::DynTypedNode Node;
BoundNodesTreeBuilder BoundNodes;
+ ast_type_traits::TraversalKin
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
aaron.ballman added inline comments.
Comment at: include/clang/AST/ASTContext.h:562-563
public:
+ ast_type_traits::TraversalKind GetTraversalKind() const { return Traversal; }
+ void SetTraversalKind(ast_type_traits::TraversalKind TK) { Traversal = TK; }
+
steveire wrote:
> aaron.ballman wrote:
> > This doesn't match our coding guidelines. Should be `getTraversalKind()`,
> > etc. Same below.
> I think clang still uses uppercase names everywhere. Can you be more specific?
No, Clang doesn't use uppercase names everywhere -- we're consistently
inconsistent and it depends mostly on the age of when the code was introduced
and what the surrounding code looks like. We still follow the usual coding
style guidelines for naming conventions -- stick with the convention used by
nearby code if it's already consistent, otherwise follow the coding style rules.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:716
+/// \endcode
+/// matches the return statement with "ret" bound to "a".
+template
aaron.ballman wrote:
> Copy pasta?
You dropped the interesting bit from the documentation here -- you should add
in what the matcher is matching (which makes the preceding "The matcher \code
... \endcode" grammatical again).
Comment at: include/clang/ASTMatchers/ASTMatchers.h:718
+template
+internal::Matcher traverse(ast_type_traits::TraversalKind TK,
+ const internal::Matcher &InnerMatcher) {
steveire wrote:
> aaron.ballman wrote:
> > Is this an API we should be exposing to clang-query as well? Will those
> > users be able to use a string literal for the traversal kind, like they
> > already do for attribute kinds (etc)?
> Yes, I thought about that, but in a follow-up patch. First, I aim to extend
> the `TraversalKind` enum with `TK_IgnoreInvisble`.
This is new functionality, so why do you want to wait for a follow-up patch (is
it somehow more involved)? We typically add support for dynamic matchers at the
same time we add support for the static matchers because otherwise the two get
frustratingly out of sync.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire added inline comments.
Comment at: include/clang/AST/ASTContext.h:562-563
public:
+ ast_type_traits::TraversalKind GetTraversalKind() const { return Traversal; }
+ void SetTraversalKind(ast_type_traits::TraversalKind TK) { Traversal = TK; }
+
aaron.ballman wrote:
> This doesn't match our coding guidelines. Should be `getTraversalKind()`,
> etc. Same below.
I think clang still uses uppercase names everywhere. Can you be more specific?
Comment at: include/clang/AST/ASTContext.h:565-568
+ const Expr *TraverseIgnored(const Expr *E);
+ Expr *TraverseIgnored(Expr *E);
+ ast_type_traits::DynTypedNode
+ TraverseIgnored(const ast_type_traits::DynTypedNode &N);
aaron.ballman wrote:
> Is there a reason we don't want these functions to be marked `const`?
It looks like they can be const.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:718
+template
+internal::Matcher traverse(ast_type_traits::TraversalKind TK,
+ const internal::Matcher &InnerMatcher) {
aaron.ballman wrote:
> Is this an API we should be exposing to clang-query as well? Will those users
> be able to use a string literal for the traversal kind, like they already do
> for attribute kinds (etc)?
Yes, I thought about that, but in a follow-up patch. First, I aim to extend the
`TraversalKind` enum with `TK_IgnoreInvisble`.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire updated this revision to Diff 199875.
steveire marked 10 inline comments as done.
steveire added a comment.
Update
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
Files:
include/clang/AST/ASTContext.h
include/clang/AST/ASTNodeTraverser.h
include/clang/ASTMatchers/ASTMatchers.h
include/clang/ASTMatchers/ASTMatchersInternal.h
lib/AST/ASTContext.cpp
lib/ASTMatchers/ASTMatchFinder.cpp
lib/ASTMatchers/ASTMatchersInternal.cpp
unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1510,6 +1510,72 @@
notMatches("class C {}; C a = C();", varDecl(has(cxxConstructExpr();
}
+TEST(Traversal, traverseMatcher) {
+
+ StringRef VarDeclCode = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+)cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ EXPECT_TRUE(
+ notMatches(VarDeclCode, traverse(ast_type_traits::TK_AsIs, Matcher)));
+ EXPECT_TRUE(
+ matches(VarDeclCode,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ Matcher)));
+}
+
+TEST(Traversal, traverseMatcherNesting) {
+
+ StringRef Code = R"cpp(
+float bar(int i)
+{
+ return i;
+}
+
+void foo()
+{
+ bar(bar(3.0));
+}
+)cpp";
+
+ EXPECT_TRUE(matches(
+ Code,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ callExpr(has(callExpr(traverse(
+ ast_type_traits::TK_AsIs,
+ callExpr(has(implicitCastExpr(has(floatLiteral(;
+}
+
+TEST(Traversal, traverseMatcherThroughMemoization) {
+
+ StringRef Code = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+ )cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ // Matchers such as hasDescendant memoize their result regarding AST
+ // nodes. In the matcher below, the first use of hasDescendant(Matcher)
+ // fails, and the use of it inside the traverse() matcher should pass
+ // causing the overall matcher to be a true match.
+ // This test verifies that the first false result is not re-used, which
+ // would cause the overall matcher to be incorrectly false.
+
+ EXPECT_TRUE(matches(
+ Code, functionDecl(anyOf(
+hasDescendant(Matcher),
+traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ functionDecl(hasDescendant(Matcher)));
+}
+
TEST(IgnoringImpCasts, MatchesImpCasts) {
// This test checks that ignoringImpCasts matches when implicit casts are
// present and its inner matcher alone does not match.
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -211,10 +211,19 @@
bool DynTypedMatcher::matches(const ast_type_traits::DynTypedNode &DynNode,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto PreviousTraversalKind = Finder->getASTContext().GetTraversalKind();
+ auto OptTK = Implementation->TraversalKind();
+ if (OptTK)
+Finder->getASTContext().SetTraversalKind(*OptTK);
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ if (RestrictKind.isBaseOf(NodeKind) &&
+ Implementation->dynMatches(N, Finder, Builder)) {
+Finder->getASTContext().SetTraversalKind(PreviousTraversalKind);
return true;
}
+ Finder->getASTContext().SetTraversalKind(PreviousTraversalKind);
// Delete all bindings when a matcher does not match.
// This prevents unexpected exposure of bound nodes in unmatches
// branches of the match tree.
@@ -225,8 +234,11 @@
bool DynTypedMatcher::matchesNoKindCheck(
const ast_type_traits::DynTypedNode &DynNode, ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
- if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
return true;
}
// Delete all bindings when a matcher does not match.
Index: lib/ASTMatchers/ASTMatchFinder.cpp
===
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -59,10 +59,12 @@
DynTypedMatcher::MatcherIDType MatcherID;
ast_type_traits::DynTypedNode Node;
BoundNodesTreeBuilder
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
aaron.ballman added a comment.
Thanks for this -- it looks like really interesting functionality! I've mostly
found nits thus far, but did have a question about clang-query support for it.
Comment at: include/clang/AST/ASTContext.h:562-563
public:
+ ast_type_traits::TraversalKind GetTraversalKind() const { return Traversal; }
+ void SetTraversalKind(ast_type_traits::TraversalKind TK) { Traversal = TK; }
+
This doesn't match our coding guidelines. Should be `getTraversalKind()`, etc.
Same below.
Comment at: include/clang/AST/ASTContext.h:565-568
+ const Expr *TraverseIgnored(const Expr *E);
+ Expr *TraverseIgnored(Expr *E);
+ ast_type_traits::DynTypedNode
+ TraverseIgnored(const ast_type_traits::DynTypedNode &N);
Is there a reason we don't want these functions to be marked `const`?
Comment at: include/clang/AST/ASTNodeTraverser.h:80
+ void SetTraversalKind(ast_type_traits::TraversalKind TK) { Traversal = TK; }
+
`setTraversalKind()`
Comment at: include/clang/AST/ASTNodeTraverser.h:105
- void Visit(const Stmt *S, StringRef Label = {}) {
+ void Visit(const Stmt *S_, StringRef Label = {}) {
getNodeDelegate().AddChild(Label, [=] {
Let's not make underscores important like this -- how about `Node` or something
generic instead?
Comment at: include/clang/AST/ASTNodeTraverser.h:113-114
+ break;
+case ast_type_traits::TK_IgnoreImplicitCastsAndParentheses:
+ S = E->IgnoreParenImpCasts();
+ break;
What an unfortunate name for this. `IgnoreParenImpCasts()` ignores parens,
implicit casts, full expressions, temporary materialization, and non-type
template substitutions, and those extra nodes have surprised people in the past.
Not much to be done about it here, just loudly lamenting the existing name of
the trait.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:701
+/// Causes all nested matchers to be matched with the specified traversal kind
+///
Missing a full stop at the end of the comment.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:716
+/// \endcode
+/// matches the return statement with "ret" bound to "a".
+template
Copy pasta?
Comment at: include/clang/ASTMatchers/ASTMatchers.h:718
+template
+internal::Matcher traverse(ast_type_traits::TraversalKind TK,
+ const internal::Matcher &InnerMatcher) {
Is this an API we should be exposing to clang-query as well? Will those users
be able to use a string literal for the traversal kind, like they already do
for attribute kinds (etc)?
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:286
+
+ virtual llvm::Optional TraversalKind() const
{
+return {};
`traversalKind()`
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:287
+ virtual llvm::Optional TraversalKind() const
{
+return {};
+ }
`return llvm::None;`
Comment at: lib/AST/ASTContext.cpp:120
+ASTContext::TraverseIgnored(const ast_type_traits::DynTypedNode &N) {
+ if (auto E = N.get()) {
+return ast_type_traits::DynTypedNode::create(*TraverseIgnored(E));
`auto *`
Comment at: lib/AST/ASTContext.cpp:10358
bool TraverseStmt(Stmt *StmtNode) {
+auto FilteredNode = StmtNode;
+if (auto *ExprNode = dyn_cast_or_null(FilteredNode))
`Stmt *`
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:148
Stmt *StmtToTraverse = StmtNode;
+if (Expr *ExprNode = dyn_cast_or_null(StmtNode))
+ StmtToTraverse = Finder->getASTContext().TraverseIgnored(ExprNode);
`auto *`
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:214-215
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto PreviousTraversalKind = Finder->getASTContext().GetTraversalKind();
+ auto OptTK = Implementation->TraversalKind();
+ if (OptTK)
Please do not use `auto` here.
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:218-219
+Finder->getASTContext().SetTraversalKind(*OptTK);
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
Or here...
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:237-238
BoundNodesTreeBuilder *Builder) const {
- assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
- if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire updated this revision to Diff 199185.
steveire added a comment.
Format
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
Files:
include/clang/AST/ASTContext.h
include/clang/AST/ASTNodeTraverser.h
include/clang/ASTMatchers/ASTMatchers.h
include/clang/ASTMatchers/ASTMatchersInternal.h
lib/AST/ASTContext.cpp
lib/ASTMatchers/ASTMatchFinder.cpp
lib/ASTMatchers/ASTMatchersInternal.cpp
unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1510,6 +1510,72 @@
notMatches("class C {}; C a = C();", varDecl(has(cxxConstructExpr();
}
+TEST(Traversal, traverseMatcher) {
+
+ StringRef VarDeclCode = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+)cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ EXPECT_TRUE(
+ notMatches(VarDeclCode, traverse(ast_type_traits::TK_AsIs, Matcher)));
+ EXPECT_TRUE(
+ matches(VarDeclCode,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ Matcher)));
+}
+
+TEST(Traversal, traverseMatcherNesting) {
+
+ StringRef Code = R"cpp(
+float bar(int i)
+{
+ return i;
+}
+
+void foo()
+{
+ bar(bar(3.0));
+}
+)cpp";
+
+ EXPECT_TRUE(matches(
+ Code,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ callExpr(has(callExpr(traverse(
+ ast_type_traits::TK_AsIs,
+ callExpr(has(implicitCastExpr(has(floatLiteral(;
+}
+
+TEST(Traversal, traverseMatcherThroughMemoization) {
+
+ StringRef Code = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+ )cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ // Matchers such as hasDescendant memoize their result regarding AST
+ // nodes. In the matcher below, the first use of hasDescendant(Matcher)
+ // fails, and the use of it inside the traverse() matcher should pass
+ // causing the overall matcher to be a true match.
+ // This test verifies that the first false result is not re-used, which
+ // would cause the overall matcher to be incorrectly false.
+
+ EXPECT_TRUE(matches(
+ Code, functionDecl(anyOf(
+hasDescendant(Matcher),
+traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ functionDecl(hasDescendant(Matcher)));
+}
+
TEST(IgnoringImpCasts, MatchesImpCasts) {
// This test checks that ignoringImpCasts matches when implicit casts are
// present and its inner matcher alone does not match.
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -211,10 +211,19 @@
bool DynTypedMatcher::matches(const ast_type_traits::DynTypedNode &DynNode,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto PreviousTraversalKind = Finder->getASTContext().GetTraversalKind();
+ auto OptTK = Implementation->TraversalKind();
+ if (OptTK)
+Finder->getASTContext().SetTraversalKind(*OptTK);
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ if (RestrictKind.isBaseOf(NodeKind) &&
+ Implementation->dynMatches(N, Finder, Builder)) {
+Finder->getASTContext().SetTraversalKind(PreviousTraversalKind);
return true;
}
+ Finder->getASTContext().SetTraversalKind(PreviousTraversalKind);
// Delete all bindings when a matcher does not match.
// This prevents unexpected exposure of bound nodes in unmatches
// branches of the match tree.
@@ -225,8 +234,11 @@
bool DynTypedMatcher::matchesNoKindCheck(
const ast_type_traits::DynTypedNode &DynNode, ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
- if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
return true;
}
// Delete all bindings when a matcher does not match.
Index: lib/ASTMatchers/ASTMatchFinder.cpp
===
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -59,10 +59,12 @@
DynTypedMatcher::MatcherIDType MatcherID;
ast_type_traits::DynTypedNode Node;
BoundNodesTreeBuilder BoundNodes;
+ ast_type_traits::TraversalKin
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire updated this revision to Diff 199181.
steveire added a comment.
Format
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
Files:
include/clang/AST/ASTContext.h
include/clang/AST/ASTNodeTraverser.h
include/clang/ASTMatchers/ASTMatchers.h
include/clang/ASTMatchers/ASTMatchersInternal.h
lib/AST/ASTContext.cpp
lib/ASTMatchers/ASTMatchFinder.cpp
lib/ASTMatchers/ASTMatchersInternal.cpp
unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1510,6 +1510,72 @@
notMatches("class C {}; C a = C();", varDecl(has(cxxConstructExpr();
}
+TEST(Traversal, traverseMatcher) {
+
+ StringRef VarDeclCode = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+)cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ EXPECT_TRUE(
+ notMatches(VarDeclCode, traverse(ast_type_traits::TK_AsIs, Matcher)));
+ EXPECT_TRUE(
+ matches(VarDeclCode,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ Matcher)));
+}
+
+TEST(Traversal, traverseMatcherNesting) {
+
+ StringRef Code = R"cpp(
+float bar(int i)
+{
+ return i;
+}
+
+void foo()
+{
+ bar(bar(3.0));
+}
+)cpp";
+
+ EXPECT_TRUE(matches(
+ Code,
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ callExpr(has(callExpr(traverse(
+ ast_type_traits::TK_AsIs,
+ callExpr(has(implicitCastExpr(has(floatLiteral(;
+}
+
+TEST(Traversal, traverseMatcherThroughMemoization) {
+
+ StringRef Code = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+ )cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ // Matchers such as hasDescendant memoize their result regarding AST
+ // nodes. In the matcher below, the first use of hasDescendant(Matcher)
+ // fails, and the use of it inside the traverse() matcher should pass
+ // causing the overall matcher to be a true match.
+ // This test verifies that the first false result is not re-used, which
+ // would cause the overall matcher to be incorrectly false.
+
+ EXPECT_TRUE(matches(
+ Code, functionDecl(anyOf(
+hasDescendant(Matcher),
+traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ functionDecl(hasDescendant(Matcher)));
+}
+
TEST(IgnoringImpCasts, MatchesImpCasts) {
// This test checks that ignoringImpCasts matches when implicit casts are
// present and its inner matcher alone does not match.
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -211,10 +211,17 @@
bool DynTypedMatcher::matches(const ast_type_traits::DynTypedNode &DynNode,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto PreviousTraversalKind = Finder->getASTContext().GetTraversalKind();
+ Finder->getASTContext().SetTraversalKind(Implementation->TraversalKind());
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ if (RestrictKind.isBaseOf(NodeKind) &&
+ Implementation->dynMatches(N, Finder, Builder)) {
+Finder->getASTContext().SetTraversalKind(PreviousTraversalKind);
return true;
}
+ Finder->getASTContext().SetTraversalKind(PreviousTraversalKind);
// Delete all bindings when a matcher does not match.
// This prevents unexpected exposure of bound nodes in unmatches
// branches of the match tree.
@@ -225,8 +232,11 @@
bool DynTypedMatcher::matchesNoKindCheck(
const ast_type_traits::DynTypedNode &DynNode, ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
- if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
return true;
}
// Delete all bindings when a matcher does not match.
Index: lib/ASTMatchers/ASTMatchFinder.cpp
===
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -59,10 +59,12 @@
DynTypedMatcher::MatcherIDType MatcherID;
ast_type_traits::DynTypedNode Node;
BoundNodesTreeBuilder BoundNodes;
+ ast_type_traits::TraversalKind Traversal = ast_type_traits::TK_AsIs;
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire added a comment. @klimek This includes a test for the memoization case you were interested in at EuroLLVM. In this iteration of the API, the traversal kind is changed by using a new `traverse` matcher, which gives the user control over whether invisible/implicit nodes are skipped or not. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61837/new/ https://reviews.llvm.org/D61837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
steveire created this revision.
steveire added reviewers: klimek, aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This will eventually allow traversal of an AST while ignoring invisible
AST nodes. Currently it depends on the available enum values for
TraversalKinds. That can be extended to ignore all invisible nodes in
the future.
Repository:
rC Clang
https://reviews.llvm.org/D61837
Files:
include/clang/AST/ASTContext.h
include/clang/AST/ASTNodeTraverser.h
include/clang/ASTMatchers/ASTMatchers.h
include/clang/ASTMatchers/ASTMatchersInternal.h
lib/AST/ASTContext.cpp
lib/ASTMatchers/ASTMatchFinder.cpp
lib/ASTMatchers/ASTMatchersInternal.cpp
unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1510,6 +1510,96 @@
notMatches("class C {}; C a = C();", varDecl(has(cxxConstructExpr();
}
+TEST(Traversal, traverseMatcher) {
+
+ StringRef VarDeclCode = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+)cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ EXPECT_TRUE(notMatches(
+VarDeclCode,
+traverse(ast_type_traits::TK_AsIs,
+ Matcher
+ )
+ ));
+ EXPECT_TRUE(matches(
+VarDeclCode,
+traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ Matcher
+ )
+ ));
+}
+
+TEST(Traversal, traverseMatcherNesting) {
+
+ StringRef Code = R"cpp(
+float bar(int i)
+{
+ return i;
+}
+
+void foo()
+{
+ bar(bar(3.0));
+}
+)cpp";
+
+ EXPECT_TRUE(matches(
+Code,
+traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ callExpr(has(
+callExpr(
+ traverse(ast_type_traits::TK_AsIs,
+callExpr(has(implicitCastExpr(has(floatLiteral()
+)
+ )
+))
+ )
+ ));
+}
+
+TEST(Traversal, traverseMatcherThroughMemoization) {
+
+ StringRef Code = R"cpp(
+void foo()
+{
+ int i = 3.0;
+}
+ )cpp";
+
+ auto Matcher = varDecl(hasInitializer(floatLiteral()));
+
+ // Matchers such as hasDescendant memoize their result regarding AST
+ // nodes. In the matcher below, the first use of hasDescendant(Matcher)
+ // fails, and the use of it inside the traverse() matcher should pass
+ // causing the overall matcher to be a true match.
+ // This test verifies that the first false result is not re-used, which
+ // would cause the overall matcher to be incorrectly false.
+
+ EXPECT_TRUE(
+matches(
+Code,
+functionDecl(anyOf(
+ hasDescendant(
+Matcher
+),
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+functionDecl(
+ hasDescendant(
+Matcher
+)
+ )
+)
+ ))
+)
+ );
+}
+
TEST(IgnoringImpCasts, MatchesImpCasts) {
// This test checks that ignoringImpCasts matches when implicit casts are
// present and its inner matcher alone does not match.
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -211,8 +211,11 @@
bool DynTypedMatcher::matches(const ast_type_traits::DynTypedNode &DynNode,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ if (RestrictKind.isBaseOf(NodeKind) &&
+ Implementation->dynMatches(N, Finder, Builder)) {
return true;
}
// Delete all bindings when a matcher does not match.
@@ -225,8 +228,11 @@
bool DynTypedMatcher::matchesNoKindCheck(
const ast_type_traits::DynTypedNode &DynNode, ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
- if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
return true;
}
// Delete all bindings when a matcher does not match.
Index: lib/ASTMatchers/ASTMatchFinder.cpp
===
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -59,10 +59,11 @@
DynTypedMatcher::MatcherIDType MatcherID;
ast_type_traits::DynTypedNode Node;
BoundNodesTreeBuilder BoundNodes;
+ ast_type_traits::TraversalKind Traversal = ast_type_traits::TK_AsIs;
bool operator<(const MatchKey &Other) const {
-return
