- Prevent transformation of loops where iterator dereferences to an rvalue
reference.
- Added comment explaining why rvalue references are used if the
iterator's operator*() returns by value.
Hi gribozavr, klimek,
http://llvm-reviews.chandlerc.com/D500
CHANGE SINCE LAST DIFF
http://llvm-reviews.chandlerc.com/D500?vs=1199&id=1206#toc
BRANCH
loop_const
ARCANIST PROJECT
clang-tools-extra
Files:
cpp11-migrate/LoopConvert/LoopActions.cpp
cpp11-migrate/LoopConvert/LoopActions.h
cpp11-migrate/LoopConvert/LoopMatchers.cpp
cpp11-migrate/LoopConvert/LoopMatchers.h
test/cpp11-migrate/LoopConvert/Inputs/structures.h
test/cpp11-migrate/LoopConvert/iterator.cpp
test/cpp11-migrate/LoopConvert/iterator_failing.cpp
Index: cpp11-migrate/LoopConvert/LoopActions.cpp
===================================================================
--- cpp11-migrate/LoopConvert/LoopActions.cpp
+++ cpp11-migrate/LoopConvert/LoopActions.cpp
@@ -734,7 +734,8 @@
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 @@
// 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 @@
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 @@
doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr),
ContainerString, Finder.getUsages(),
- Finder.getAliasDecl(), TheLoop, ContainerNeedsDereference);
+ Finder.getAliasDecl(), TheLoop, ContainerNeedsDereference,
+ DerefByValue);
++*AcceptedChanges;
}
@@ -986,6 +996,9 @@
if (!ContainerExpr && !BoundExpr)
return;
+ bool DerefByValue = Nodes.getNodeAs<QualType>(DerefByValueResultName) != 0;
+
findAndVerifyUsages(Context, LoopVar, EndVar, ContainerExpr, BoundExpr,
- ContainerNeedsDereference, TheLoop, ConfidenceLevel);
+ ContainerNeedsDereference, DerefByValue, TheLoop,
+ ConfidenceLevel);
}
Index: cpp11-migrate/LoopConvert/LoopActions.h
===================================================================
--- cpp11-migrate/LoopConvert/LoopActions.h
+++ cpp11-migrate/LoopConvert/LoopActions.h
@@ -74,7 +74,8 @@
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 @@
const clang::Expr *ContainerExpr,
const clang::Expr *BoundExpr,
bool ContainerNeedsDereference,
+ bool DerefByValue,
const clang::ForStmt *TheLoop,
Confidence ConfidenceLevel);
Index: cpp11-migrate/LoopConvert/LoopMatchers.cpp
===================================================================
--- cpp11-migrate/LoopConvert/LoopMatchers.cpp
+++ cpp11-migrate/LoopConvert/LoopMatchers.cpp
@@ -25,6 +25,7 @@
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 @@
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<VarDecl> 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).
Index: cpp11-migrate/LoopConvert/LoopMatchers.h
===================================================================
--- cpp11-migrate/LoopConvert/LoopMatchers.h
+++ cpp11-migrate/LoopConvert/LoopMatchers.h
@@ -30,6 +30,7 @@
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();
Index: test/cpp11-migrate/LoopConvert/Inputs/structures.h
===================================================================
--- test/cpp11-migrate/LoopConvert/Inputs/structures.h
+++ test/cpp11-migrate/LoopConvert/Inputs/structures.h
@@ -150,4 +150,27 @@
iterator end() const;
};
+template <typename T>
+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 <typename T>
+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
Index: test/cpp11-migrate/LoopConvert/iterator.cpp
===================================================================
--- test/cpp11-migrate/LoopConvert/iterator.cpp
+++ 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 @@
}
// 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*> int_ptrs;
+ for (PtrSet<int*>::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> int_ptrs;
+ for (TypedefDerefContainer<int>::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<int> container;
+ for (RValueDerefContainer<int>::iterator I = container.begin(),
+ E = container.end(); I != E; ++I) {
+ // CHECK: for (RValueDerefContainer<int>::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.
Index: test/cpp11-migrate/LoopConvert/iterator_failing.cpp
===================================================================
--- 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*> int_ptrs;
- for (PtrSet<int*>::iterator I = int_ptrs.begin(),
- E = int_ptrs.end(); I != E; ++I) {
- // CHECK: for (const auto & int_ptr : int_ptrs) {
- }
-}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
