From 4f05d7143f5cf831e39e5ed16d33a19dc6da9dcd Mon Sep 17 00:00:00 2001 From: Edwin Vane Date: Thu, 7 Mar 2013 16:22:05 +0000 Subject: [PATCH] Have LoopConvert use 'auto &&' where necessary For iterators where the dereference operator returns by value, LoopConvert should use 'auto &&' in the range-based for loop expression. If the dereference operator returns an rvalue reference, this is deemed too strange and the for loop is not converted. Moved test case from iterator_failing.cpp to iterator.cpp and added extra tests. Fixes PR15437. Reviewer: gribozavr llvm-svn: 176631 --- .../cpp11-migrate/LoopConvert/LoopActions.cpp | 23 +++-- .../cpp11-migrate/LoopConvert/LoopActions.h | 4 +- .../LoopConvert/LoopMatchers.cpp | 88 ++++++++++++++----- .../cpp11-migrate/LoopConvert/LoopMatchers.h | 1 + .../LoopConvert/Inputs/structures.h | 23 +++++ .../cpp11-migrate/LoopConvert/iterator.cpp | 33 ++++++- .../LoopConvert/iterator_failing.cpp | 14 --- 7 files changed, 144 insertions(+), 42 deletions(-) delete mode 100644 clang-tools-extra/test/cpp11-migrate/LoopConvert/iterator_failing.cpp diff --git a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.cpp b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.cpp index 1a700706f759..a4afe7c9f92d 100644 --- a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.cpp +++ b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.cpp @@ -734,7 +734,8 @@ void LoopFixer::doConversion(ASTContext *Context, StringRef ContainerString, const UsageResult &Usages, const DeclStmt *AliasDecl, const ForStmt *TheLoop, - bool ContainerNeedsDereference) { + bool ContainerNeedsDereference, + bool DerefByValue) { std::string VarName; if (Usages.size() == 1 && AliasDecl) { @@ -767,8 +768,15 @@ void LoopFixer::doConversion(ASTContext *Context, // Now, we need to construct the new range expresion. SourceRange ParenRange(TheLoop->getLParenLoc(), TheLoop->getRParenLoc()); - QualType AutoRefType = - Context->getLValueReferenceType(Context->getAutoDeductType()); + QualType AutoRefType = Context->getAutoDeductType(); + + // If an iterator's operator*() returns a 'T&' we can bind that to 'auto&'. + // If operator*() returns 'T' we can bind that to 'auto&&' which will deduce + // to 'T&&'. + if (DerefByValue) + AutoRefType = Context->getRValueReferenceType(AutoRefType); + else + AutoRefType = Context->getLValueReferenceType(AutoRefType); std::string MaybeDereference = ContainerNeedsDereference ? "*" : ""; std::string TypeString = AutoRefType.getAsString(); @@ -895,6 +903,7 @@ void LoopFixer::findAndVerifyUsages(ASTContext *Context, const Expr *ContainerExpr, const Expr *BoundExpr, bool ContainerNeedsDereference, + bool DerefByValue, const ForStmt *TheLoop, Confidence ConfidenceLevel) { ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr, @@ -928,7 +937,8 @@ void LoopFixer::findAndVerifyUsages(ASTContext *Context, doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr), ContainerString, Finder.getUsages(), - Finder.getAliasDecl(), TheLoop, ContainerNeedsDereference); + Finder.getAliasDecl(), TheLoop, ContainerNeedsDereference, + DerefByValue); ++*AcceptedChanges; } @@ -986,6 +996,9 @@ void LoopFixer::run(const MatchFinder::MatchResult &Result) { if (!ContainerExpr && !BoundExpr) return; + bool DerefByValue = Nodes.getNodeAs(DerefByValueResultName) != 0; + findAndVerifyUsages(Context, LoopVar, EndVar, ContainerExpr, BoundExpr, - ContainerNeedsDereference, TheLoop, ConfidenceLevel); + ContainerNeedsDereference, DerefByValue, TheLoop, + ConfidenceLevel); } diff --git a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.h b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.h index 504e6ab64d27..bd91a98f7bd0 100644 --- a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.h +++ b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.h @@ -74,7 +74,8 @@ class LoopFixer : public clang::ast_matchers::MatchFinder::MatchCallback { const UsageResult &Usages, const clang::DeclStmt *AliasDecl, const clang::ForStmt *TheLoop, - bool ContainerNeedsDereference); + bool ContainerNeedsDereference, + bool DerefByValue); /// \brief Given a loop header that would be convertible, discover all usages /// of the index variable and convert the loop if possible. @@ -84,6 +85,7 @@ class LoopFixer : public clang::ast_matchers::MatchFinder::MatchCallback { const clang::Expr *ContainerExpr, const clang::Expr *BoundExpr, bool ContainerNeedsDereference, + bool DerefByValue, const clang::ForStmt *TheLoop, Confidence ConfidenceLevel); diff --git a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.cpp b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.cpp index 1b6f59c8573d..ffd9f794b0cf 100644 --- a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.cpp +++ b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.cpp @@ -25,6 +25,7 @@ const char InitVarName[] = "initVar"; const char EndCallName[] = "endCall"; const char ConditionEndVarName[] = "conditionEndVar"; const char EndVarName[] = "endVar"; +const char DerefByValueResultName[] = "derefByValueResult"; // shared matchers static const TypeMatcher AnyType = anything(); @@ -137,30 +138,75 @@ StatementMatcher makeIteratorLoopMatcher() { hasArgument(0, IteratorComparisonMatcher), hasArgument(1, IteratorBoundMatcher)); - return forStmt( + // This matcher tests that a declaration is a CXXRecordDecl that has an + // overloaded operator*(). If the operator*() returns by value instead of by + // reference then the return type is tagged with DerefByValueResultName. + internal::Matcher TestDerefReturnsByValue = + hasType( + recordDecl( + hasMethod( + allOf( + hasOverloadedOperatorName("*"), + anyOf( + // Tag the return type if it's by value. + returns( + qualType( + unless(hasCanonicalType(referenceType())) + ).bind(DerefByValueResultName) + ), + returns( + // Skip loops where the iterator's operator* returns an + // rvalue reference. This is just weird. + qualType(unless(hasCanonicalType(rValueReferenceType()))) + ) + ) + ) + ) + ) + ); + + return + forStmt( hasLoopInit(anyOf( - declStmt(declCountIs(2), - containsDeclaration(0, InitDeclMatcher), - containsDeclaration(1, EndDeclMatcher)), - declStmt(hasSingleDecl(InitDeclMatcher)))), + declStmt( + declCountIs(2), + containsDeclaration(0, InitDeclMatcher), + containsDeclaration(1, EndDeclMatcher) + ), + declStmt(hasSingleDecl(InitDeclMatcher)) + )), hasCondition(anyOf( - binaryOperator(hasOperatorName("!="), - hasLHS(IteratorComparisonMatcher), - hasRHS(IteratorBoundMatcher)), - binaryOperator(hasOperatorName("!="), - hasLHS(IteratorBoundMatcher), - hasRHS(IteratorComparisonMatcher)), - OverloadedNEQMatcher)), + binaryOperator( + hasOperatorName("!="), + hasLHS(IteratorComparisonMatcher), + hasRHS(IteratorBoundMatcher) + ), + binaryOperator( + hasOperatorName("!="), + hasLHS(IteratorBoundMatcher), + hasRHS(IteratorComparisonMatcher) + ), + OverloadedNEQMatcher + )), hasIncrement(anyOf( - unaryOperator(hasOperatorName("++"), - hasUnaryOperand(declRefExpr(to( - varDecl(hasType(pointsTo(AnyType))) - .bind(IncrementVarName))))), - operatorCallExpr( - hasOverloadedOperatorName("++"), - hasArgument(0, declRefExpr(to( - varDecl().bind(IncrementVarName)))))))) - .bind(LoopName); + unaryOperator( + hasOperatorName("++"), + hasUnaryOperand( + declRefExpr(to( + varDecl(hasType(pointsTo(AnyType))).bind(IncrementVarName) + )) + ) + ), + operatorCallExpr( + hasOverloadedOperatorName("++"), + hasArgument(0, + declRefExpr(to( + varDecl(TestDerefReturnsByValue).bind(IncrementVarName) + )) + ) + ) + )) + ).bind(LoopName); } /// \brief The matcher used for array-like containers (pseudoarrays). diff --git a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.h b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.h index 4a3989c023ee..7824a82ad48c 100644 --- a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.h +++ b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.h @@ -30,6 +30,7 @@ extern const char InitVarName[]; extern const char EndExprName[]; extern const char EndCallName[]; extern const char EndVarName[]; +extern const char DerefByValueResultName[]; clang::ast_matchers::StatementMatcher makeArrayLoopMatcher(); clang::ast_matchers::StatementMatcher makeIteratorLoopMatcher(); diff --git a/clang-tools-extra/test/cpp11-migrate/LoopConvert/Inputs/structures.h b/clang-tools-extra/test/cpp11-migrate/LoopConvert/Inputs/structures.h index 3379b05ac1e0..e37f2b54794a 100644 --- a/clang-tools-extra/test/cpp11-migrate/LoopConvert/Inputs/structures.h +++ b/clang-tools-extra/test/cpp11-migrate/LoopConvert/Inputs/structures.h @@ -150,4 +150,27 @@ struct PtrSet { iterator end() const; }; +template +struct TypedefDerefContainer { + struct iterator { + typedef T &deref_type; + bool operator!=(const iterator &other) const; + deref_type operator*(); + iterator &operator++(); + }; + iterator begin() const; + iterator end() const; +}; + +template +struct RValueDerefContainer { + struct iterator { + typedef T &&deref_type; + bool operator!=(const iterator &other) const; + deref_type operator*(); + iterator &operator++(); + }; + iterator begin() const; + iterator end() const; +}; #endif // STRUCTURES_H diff --git a/clang-tools-extra/test/cpp11-migrate/LoopConvert/iterator.cpp b/clang-tools-extra/test/cpp11-migrate/LoopConvert/iterator.cpp index a59a90fbfa1f..fc36e07fe8b1 100644 --- a/clang-tools-extra/test/cpp11-migrate/LoopConvert/iterator.cpp +++ b/clang-tools-extra/test/cpp11-migrate/LoopConvert/iterator.cpp @@ -1,5 +1,5 @@ // RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp -// RUN: cpp11-migrate -loop-convert %t.cpp -- -I %S/Inputs +// RUN: cpp11-migrate -loop-convert %t.cpp -- -I %S/Inputs -std=c++11 // RUN: FileCheck -input-file=%t.cpp %s // RUN: cpp11-migrate -loop-convert %t.cpp -risk=risky -- -I %S/Inputs // RUN: FileCheck -check-prefix=RISKY -input-file=%t.cpp %s @@ -101,6 +101,37 @@ void f() { } // CHECK: for ({{[a-zA-Z_ ]*&? ?}}[[VAR:[a-z_]+]] : intmap) // CHECK-NEXT: printf("intmap[%d] = %d", [[VAR]].first, [[VAR]].second); + + // PtrSet's iterator dereferences by value so auto & can't be used. + { + PtrSet int_ptrs; + for (PtrSet::iterator I = int_ptrs.begin(), + E = int_ptrs.end(); I != E; ++I) { + // CHECK: for (auto && int_ptr : int_ptrs) { + } + } + + // This container uses an iterator where the derefence type is a typedef of + // a reference type. Make sure non-const auto & is still used. A failure here + // means canonical types aren't being tested. + { + TypedefDerefContainer int_ptrs; + for (TypedefDerefContainer::iterator I = int_ptrs.begin(), + E = int_ptrs.end(); I != E; ++I) { + // CHECK: for (auto & int_ptr : int_ptrs) { + } + } + + { + // Iterators returning an rvalue reference should disqualify the loop from + // transformation. + RValueDerefContainer container; + for (RValueDerefContainer::iterator I = container.begin(), + E = container.end(); I != E; ++I) { + // CHECK: for (RValueDerefContainer::iterator I = container.begin(), + // CHECK-NEXT: E = container.end(); I != E; ++I) { + } + } } // Tests to ensure that an implicit 'this' is picked up as the container. diff --git a/clang-tools-extra/test/cpp11-migrate/LoopConvert/iterator_failing.cpp b/clang-tools-extra/test/cpp11-migrate/LoopConvert/iterator_failing.cpp deleted file mode 100644 index 5684497f7458..000000000000 --- a/clang-tools-extra/test/cpp11-migrate/LoopConvert/iterator_failing.cpp +++ /dev/null @@ -1,14 +0,0 @@ -// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp -// RUN: cpp11-migrate -loop-convert %t.cpp -- -I %S/Inputs -// RUN: FileCheck -input-file=%t.cpp %s -// XFAIL: * -#include "structures.h" - -void f() { - // See PR15437 for details. - PtrSet int_ptrs; - for (PtrSet::iterator I = int_ptrs.begin(), - E = int_ptrs.end(); I != E; ++I) { - // CHECK: for (const auto & int_ptr : int_ptrs) { - } -}