[PATCH] D95573: [ASTMatchers] Avoid pathological traversal over nested lambdas
sammccall added a comment. Since the 12 branch has been cut, I've added a blocking bug to get this fixed in some form: https://bugs.llvm.org/show_bug.cgi?id=48935 @rsmith I've assigned it to you to make a call about whether to cherrypick this patch, this patch+followup, or revert the original change on the branch. (@steveire is a revert really an option here, or is there already more stuff relying on this?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95573/new/ https://reviews.llvm.org/D95573 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95573: [ASTMatchers] Avoid pathological traversal over nested lambdas
steveire added inline comments.
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2063-2065
+ if (const auto *MD = dyn_cast(D)) {
+if (const CXXRecordDecl *RD = MD->getParent()) {
+ if (RD->isLambda()) {
rsmith wrote:
> This is incorrectly skipping the bodies of all other lambda member functions
> (default ctor, copy ctor, destructor, implicit conversion to function pointer
> type), not only the "body" of the lambda itself (the definition of the
> `operator()`).
Please see https://reviews.llvm.org/D95644.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95573/new/
https://reviews.llvm.org/D95573
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95573: [ASTMatchers] Avoid pathological traversal over nested lambdas
rsmith added inline comments.
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2063-2065
+ if (const auto *MD = dyn_cast(D)) {
+if (const CXXRecordDecl *RD = MD->getParent()) {
+ if (RD->isLambda()) {
This is incorrectly skipping the bodies of all other lambda member functions
(default ctor, copy ctor, destructor, implicit conversion to function pointer
type), not only the "body" of the lambda itself (the definition of the
`operator()`).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95573/new/
https://reviews.llvm.org/D95573
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95573: [ASTMatchers] Avoid pathological traversal over nested lambdas
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6f0df3cddb3e: [ASTMatchers] Avoid pathological traversal
over nested lambdas (authored by stephenkelly).
Changed prior to commit:
https://reviews.llvm.org/D95573?vs=319702=319941#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95573/new/
https://reviews.llvm.org/D95573
Files:
clang/include/clang/AST/RecursiveASTVisitor.h
clang/lib/ASTMatchers/ASTMatchFinder.cpp
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -3853,6 +3853,78 @@
}
}
+TEST(IgnoringImpCasts, PathologicalLambda) {
+
+ // Test that deeply nested lambdas are not a performance penalty
+ StringRef Code = R"cpp(
+void f() {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+ [] {
+int i = 42;
+(void)i;
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+ }();
+}
+ )cpp";
+
+ EXPECT_TRUE(matches(Code, integerLiteral(equals(42;
+ EXPECT_TRUE(matches(Code, functionDecl(hasDescendant(integerLiteral(equals(42));
+}
+
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/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -556,9 +556,9 @@
if (LE->hasExplicitResultType())
TraverseTypeLoc(Proto.getReturnLoc());
TraverseStmt(LE->getTrailingRequiresClause());
-
-TraverseStmt(LE->getBody());
}
+
+ TraverseStmt(LE->getBody());
return true;
}
return RecursiveASTVisitor::dataTraverseNode(S, Queue);
@@ -697,6 +697,10 @@
bool shouldVisitTemplateInstantiations() const { return true; }
bool shouldVisitImplicitCode() const { return true; }
+ // We visit the lambda body explicitly, so instruct the RAV
+ // to not visit it on our behalf too.
+ bool shouldVisitLambdaBody() const { return false; }
+
bool IsMatchingInASTNodeNotSpelledInSource() const override {
return TraversingASTNodeNotSpelledInSource;
}
Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -186,6 +186,9 @@
/// code, e.g., implicit constructors and destructors.
bool shouldVisitImplicitCode() const { return false; }
+ /// Return whether this visitor should recurse into lambda body
+ bool shouldVisitLambdaBody() const { return true; }
+
/// Return whether this visitor should traverse post-order.
bool shouldTraversePostOrder() const { return false; }
@@ -2057,6 +2060,14 @@
// by clang.
(!D->isDefaulted() || getDerived().shouldVisitImplicitCode());
+ if (const auto *MD = dyn_cast(D)) {
+if (const CXXRecordDecl *RD = MD->getParent()) {
+ if (RD->isLambda()) {
+VisitBody = VisitBody && getDerived().shouldVisitLambdaBody();
+ }
+}
+ }
+
if (VisitBody) {
TRY_TO(TraverseStmt(D->getBody())); // Function body.
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95573: [ASTMatchers] Avoid pathological traversal over nested lambdas
alexfh added a comment.
Thanks for the prompt fix, btw!
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2064
+ if (const auto *MD = dyn_cast(D)) {
+if (const auto *RD = MD->getParent()) {
+ if (RD->isLambda()) {
Please specify the type explicitly here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95573/new/
https://reviews.llvm.org/D95573
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95573: [ASTMatchers] Avoid pathological traversal over nested lambdas
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
This fixes the issue with exponential traversal times for deeply nested
lambdas. Please add a test though. For example, this one:
void f() {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
[] {
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}();
}
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95573/new/
https://reviews.llvm.org/D95573
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95573: [ASTMatchers] Avoid pathological traversal over nested lambdas
steveire created this revision.
steveire added a reviewer: aaron.ballman.
steveire requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D95573
Files:
clang/include/clang/AST/RecursiveASTVisitor.h
clang/lib/ASTMatchers/ASTMatchFinder.cpp
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -561,9 +561,9 @@
if (LE->hasExplicitResultType())
TraverseTypeLoc(Proto.getReturnLoc());
TraverseStmt(LE->getTrailingRequiresClause());
-
-TraverseStmt(LE->getBody());
}
+
+ TraverseStmt(LE->getBody());
return true;
}
return RecursiveASTVisitor::dataTraverseNode(S, Queue);
@@ -702,6 +702,10 @@
bool shouldVisitTemplateInstantiations() const { return true; }
bool shouldVisitImplicitCode() const { return true; }
+ // We visit the lambda body explicitly, so instruct the RAV
+ // to not visit it on our behalf too.
+ bool shouldVisitLambdaBody() const { return false; }
+
bool IsMatchingInASTNodeNotSpelledInSource() const override {
return TraversingASTNodeNotSpelledInSource;
}
Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -186,6 +186,9 @@
/// code, e.g., implicit constructors and destructors.
bool shouldVisitImplicitCode() const { return false; }
+ /// Return whether this visitor should recurse into lambda body
+ bool shouldVisitLambdaBody() const { return true; }
+
/// Return whether this visitor should traverse post-order.
bool shouldTraversePostOrder() const { return false; }
@@ -2057,6 +2060,14 @@
// by clang.
(!D->isDefaulted() || getDerived().shouldVisitImplicitCode());
+ if (const auto *MD = dyn_cast(D)) {
+if (const auto *RD = MD->getParent()) {
+ if (RD->isLambda()) {
+VisitBody = VisitBody && getDerived().shouldVisitLambdaBody();
+ }
+}
+ }
+
if (VisitBody) {
TRY_TO(TraverseStmt(D->getBody())); // Function body.
}
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -561,9 +561,9 @@
if (LE->hasExplicitResultType())
TraverseTypeLoc(Proto.getReturnLoc());
TraverseStmt(LE->getTrailingRequiresClause());
-
-TraverseStmt(LE->getBody());
}
+
+ TraverseStmt(LE->getBody());
return true;
}
return RecursiveASTVisitor::dataTraverseNode(S, Queue);
@@ -702,6 +702,10 @@
bool shouldVisitTemplateInstantiations() const { return true; }
bool shouldVisitImplicitCode() const { return true; }
+ // We visit the lambda body explicitly, so instruct the RAV
+ // to not visit it on our behalf too.
+ bool shouldVisitLambdaBody() const { return false; }
+
bool IsMatchingInASTNodeNotSpelledInSource() const override {
return TraversingASTNodeNotSpelledInSource;
}
Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -186,6 +186,9 @@
/// code, e.g., implicit constructors and destructors.
bool shouldVisitImplicitCode() const { return false; }
+ /// Return whether this visitor should recurse into lambda body
+ bool shouldVisitLambdaBody() const { return true; }
+
/// Return whether this visitor should traverse post-order.
bool shouldTraversePostOrder() const { return false; }
@@ -2057,6 +2060,14 @@
// by clang.
(!D->isDefaulted() || getDerived().shouldVisitImplicitCode());
+ if (const auto *MD = dyn_cast(D)) {
+if (const auto *RD = MD->getParent()) {
+ if (RD->isLambda()) {
+VisitBody = VisitBody && getDerived().shouldVisitLambdaBody();
+ }
+}
+ }
+
if (VisitBody) {
TRY_TO(TraverseStmt(D->getBody())); // Function body.
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
