This is an archive of the discontinued LLVM Phabricator instance.

Differential D95644

Ensure that we traverse non-op() method bodys of lambdas
ClosedPublic

Authored by steveire on Jan 28 2021, 3:43 PM.

Details

Reviewers
rsmith
aaron.ballman
Commits
rG43cc4f15008f: Ensure that we traverse non-op() method bodys of lambdas

Diff Detail

Event Timeline

steveire requested review of this revision.Jan 28 2021, 3:43 PM
steveire created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 3:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steveire added inline comments.Jan 28 2021, 3:44 PM
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
485

I don't know how to create a lambda with a default ctor body.

steveire mentioned this in D95573: [ASTMatchers] Avoid pathological traversal over nested lambdas.Jan 28 2021, 3:45 PM
steveire added a reviewer: aaron.ballman.Jan 28 2021, 3:58 PM
rsmith added inline comments.Jan 28 2021, 4:12 PM
clang/include/clang/AST/RecursiveASTVisitor.h
2065–2066

In principle there can be multiple declarations of the lambda call operator if we merge multiple lambdas from different modules; we should check for any declaration of the operator() here rather than the exact one that getLambdaCallOperator returns.

I'd previously thought we could use isLambdaCallOperator here, but I think that's wrong: for a generic lambda, that would skip instantiations of the templated operator(), whereas I think you only want to skip the body of the primary template in that case, and still visit the instantiations, right? Eg, given:

auto x = [](auto n) { return n; };
int n = x(0);

... you'd want to skip the body of the template <lambda>::operator()(T), but you'd still want to visit the body of <lambda>::operator()<int>(int), right?

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
485

I think that's probably not actually possible, sorry for misleading you on that! You can introduce a copy constructor with a body (by giving A a non-trivial copy constructor), though, if you want to test that.

steveire updated this revision to Diff 319987.Jan 28 2021, 4:17 PM

Use helper method

Harbormaster completed remote builds in B87081: Diff 319977.Jan 28 2021, 4:24 PM
steveire added inline comments.Jan 28 2021, 4:30 PM
clang/include/clang/AST/RecursiveASTVisitor.h
2065–2066

Yes, the instantiations should still be visited here.

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
485

Thanks! I think I have the copy-ctor case covered already. Is something missing?

steveire updated this revision to Diff 319992.Jan 28 2021, 4:31 PM

Update

rsmith accepted this revision.Jan 28 2021, 4:40 PM
This revision is now accepted and ready to land.Jan 28 2021, 4:40 PM
rsmith added inline comments.Jan 28 2021, 4:41 PM
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
485

Oh, right you are. Looks good :)

This revision was landed with ongoing or failed builds.Jan 28 2021, 4:49 PM
Closed by commit rG43cc4f15008f: Ensure that we traverse non-op() method bodys of lambdas (authored by • stephenkelly). · Explain Why
This revision was automatically updated to reflect the committed changes.
• stephenkelly added a commit: rG43cc4f15008f: Ensure that we traverse non-op() method bodys of lambdas.
Harbormaster completed remote builds in B87089: Diff 319987.Jan 28 2021, 5:29 PM
Harbormaster completed remote builds in B87091: Diff 319992.Jan 28 2021, 5:41 PM

Revision Contents

PathSize
clang/
include/
clang/
AST/
3 lines
unittests/
ASTMatchers/
36 lines

Diff 320000

clang/include/clang/AST/RecursiveASTVisitor.h

Show First 20 Lines • Show All 2,056 Lines • ▼ Show 20 Linesbool RecursiveASTVisitor<Derived>::TraverseFunctionHelper(FunctionDecl *D) {
bool VisitBody = bool VisitBody =
D->isThisDeclarationADefinition() && D->isThisDeclarationADefinition() &&
// Don't visit the function body if the function definition is generated // Don't visit the function body if the function definition is generated
// by clang. // by clang.
(!D->isDefaulted() || getDerived().shouldVisitImplicitCode()); (!D->isDefaulted() || getDerived().shouldVisitImplicitCode());
if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) { if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
if (const CXXRecordDecl *RD = MD->getParent()) { if (const CXXRecordDecl *RD = MD->getParent()) {
if (RD->isLambda()) { if (RD->isLambda() &&
declaresSameEntity(RD->getLambdaCallOperator(), MD)) {
rsmithUnsubmitted
Not Done
ReplyInline Actions
if (const CXXRecordDecl *RD = MD->getParent()) {
- if (RD->isLambda() && RD->getLambdaCallOperator() == MD) {
+ if (RD->isLambda() && declaresSameEntity(RD->getLambdaCallOperator(), MD)) {
VisitBody = VisitBody && getDerived().shouldVisitLambdaBody();

In principle there can be multiple declarations of the lambda call operator if we merge multiple lambdas from different modules; we should check for any declaration of the operator() here rather than the exact one that getLambdaCallOperator returns.

I'd previously thought we could use isLambdaCallOperator here, but I think that's wrong: for a generic lambda, that would skip instantiations of the templated operator(), whereas I think you only want to skip the body of the primary template in that case, and still visit the instantiations, right? Eg, given:

auto x = [](auto n) { return n; };
int n = x(0);

... you'd want to skip the body of the template <lambda>::operator()(T), but you'd still want to visit the body of <lambda>::operator()<int>(int), right?

rsmith: In principle there can be multiple declarations of the lambda call operator if we merge…
steveireAuthorUnsubmitted
Done
ReplyInline Actions

Yes, the instantiations should still be visited here.

steveire: Yes, the instantiations should still be visited here.
VisitBody = VisitBody && getDerived().shouldVisitLambdaBody(); VisitBody = VisitBody && getDerived().shouldVisitLambdaBody();
} }
} }
} }
if (VisitBody) { if (VisitBody) {
TRY_TO(TraverseStmt(D->getBody())); // Function body. TRY_TO(TraverseStmt(D->getBody())); // Function body.
} }
▲ Show 20 Lines • Show All 1,510 Lines • Show Last 20 Lines

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Show First 20 Lines • Show All 468 Lines • ▼ Show 20 LinesTEST(Matcher, CapturesThis) {
EXPECT_TRUE( EXPECT_TRUE(
matches("struct a { void f() { [this](){}; }; };", HasCaptureThis)); matches("struct a { void f() { [this](){}; }; };", HasCaptureThis));
EXPECT_TRUE(notMatches("void f() { [](){}; }", HasCaptureThis)); EXPECT_TRUE(notMatches("void f() { [](){}; }", HasCaptureThis));
EXPECT_TRUE(notMatches("void f() { int x = 3; [x](){}; }", HasCaptureThis)); EXPECT_TRUE(notMatches("void f() { int x = 3; [x](){}; }", HasCaptureThis));
EXPECT_TRUE(notMatches("void f() { int x = 3; [&x](){}; }", HasCaptureThis)); EXPECT_TRUE(notMatches("void f() { int x = 3; [&x](){}; }", HasCaptureThis));
EXPECT_TRUE(notMatches("void f() { int z = 3; [&z](){}; }", HasCaptureThis)); EXPECT_TRUE(notMatches("void f() { int z = 3; [&z](){}; }", HasCaptureThis));
} }
TEST(Matcher, MatchesMethodsOnLambda) {
StringRef Code = R"cpp(
struct A {
~A() {}
};
void foo()
{
A a;
auto l = [a] { };
steveireAuthorUnsubmitted
Done
ReplyInline Actions

I don't know how to create a lambda with a default ctor body.

steveire: I don't know how to create a lambda with a default ctor body.
rsmithUnsubmitted
Not Done
ReplyInline Actions

I think that's probably not actually possible, sorry for misleading you on that! You can introduce a copy constructor with a body (by giving A a non-trivial copy constructor), though, if you want to test that.

rsmith: I think that's probably not actually possible, sorry for misleading you on that! You can…
steveireAuthorUnsubmitted
Done
ReplyInline Actions

Thanks! I think I have the copy-ctor case covered already. Is something missing?

steveire: Thanks! I think I have the copy-ctor case covered already. Is something missing?
rsmithUnsubmitted
Not Done
ReplyInline Actions

Oh, right you are. Looks good :)

rsmith: Oh, right you are. Looks good :)
auto lCopy = l;
auto lPtrDecay = +[] { };
(void)lPtrDecay;
}
)cpp";
EXPECT_TRUE(matches(
Code, cxxConstructorDecl(
hasBody(compoundStmt()),
hasAncestor(lambdaExpr(hasAncestor(varDecl(hasName("l"))))),
isCopyConstructor())));
EXPECT_TRUE(matches(
Code, cxxConstructorDecl(
hasBody(compoundStmt()),
hasAncestor(lambdaExpr(hasAncestor(varDecl(hasName("l"))))),
isMoveConstructor())));
EXPECT_TRUE(matches(
Code, cxxDestructorDecl(
hasBody(compoundStmt()),
hasAncestor(lambdaExpr(hasAncestor(varDecl(hasName("l"))))))));
EXPECT_TRUE(matches(
Code, cxxConversionDecl(hasBody(compoundStmt(has(returnStmt(
hasReturnValue(implicitCastExpr()))))),
hasAncestor(lambdaExpr(hasAncestor(
varDecl(hasName("lPtrDecay"))))))));
}
TEST(Matcher, isClassMessage) { TEST(Matcher, isClassMessage) {
EXPECT_TRUE(matchesObjC( EXPECT_TRUE(matchesObjC(
"@interface NSString +(NSString *) stringWithFormat; @end " "@interface NSString +(NSString *) stringWithFormat; @end "
"void f() { [NSString stringWithFormat]; }", "void f() { [NSString stringWithFormat]; }",
objcMessageExpr(isClassMessage()))); objcMessageExpr(isClassMessage())));
EXPECT_FALSE(matchesObjC( EXPECT_FALSE(matchesObjC(
"@interface NSString @end " "@interface NSString @end "
▲ Show 20 Lines • Show All 4,615 Lines • Show Last 20 Lines