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

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 =
+  QualType AutoRefType = Context->getAutoDeductType();
-      Context->getLValueReferenceType(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<QualType>(DerefByValueResultName) != 0;
+
   findAndVerifyUsages(Context, LoopVar, EndVar, ContainerExpr, BoundExpr,
-                      ContainerNeedsDereference, TheLoop, ConfidenceLevel);
+                      ContainerNeedsDereference, DerefByValue, TheLoop,
+                      ConfidenceLevel);
 }

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);
 

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<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),
+        declStmt(
-                   containsDeclaration(0, InitDeclMatcher),
+          declCountIs(2),
-                   containsDeclaration(1, EndDeclMatcher)),
+          containsDeclaration(0, InitDeclMatcher),
-          declStmt(hasSingleDecl(InitDeclMatcher)))),
+          containsDeclaration(1, EndDeclMatcher)
+        ),
+        declStmt(hasSingleDecl(InitDeclMatcher))
+      )),
       hasCondition(anyOf(
-          binaryOperator(hasOperatorName("!="),
+        binaryOperator(
-                         hasLHS(IteratorComparisonMatcher),
+          hasOperatorName("!="),
-                         hasRHS(IteratorBoundMatcher)),
+          hasLHS(IteratorComparisonMatcher),
-          binaryOperator(hasOperatorName("!="),
+          hasRHS(IteratorBoundMatcher)
-                         hasLHS(IteratorBoundMatcher),
+        ),
-                         hasRHS(IteratorComparisonMatcher)),
+        binaryOperator(
-          OverloadedNEQMatcher)),
+          hasOperatorName("!="),
+          hasLHS(IteratorBoundMatcher),
+          hasRHS(IteratorComparisonMatcher)
+        ),
+        OverloadedNEQMatcher
+      )),
       hasIncrement(anyOf(
-          unaryOperator(hasOperatorName("++"),
+        unaryOperator(
-                        hasUnaryOperand(declRefExpr(to(
+          hasOperatorName("++"),
-                            varDecl(hasType(pointsTo(AnyType)))
+          hasUnaryOperand(
-                            .bind(IncrementVarName))))),
+            declRefExpr(to(
-          operatorCallExpr(
+              varDecl(hasType(pointsTo(AnyType))).bind(IncrementVarName)
-              hasOverloadedOperatorName("++"),
+            ))
-              hasArgument(0, declRefExpr(to(
+          )
-                  varDecl().bind(IncrementVarName))))))))
+        ),
-                  .bind(LoopName);
+        operatorCallExpr(
+          hasOverloadedOperatorName("++"),
+          hasArgument(0,
+            declRefExpr(to(
+              varDecl(TestDerefReturnsByValue).bind(IncrementVarName)
+            ))
+          )
+        )
+      ))
+    ).bind(LoopName);
 }
 
 /// \brief The matcher used for array-like containers (pseudoarrays).

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();

clang-tools-extra/test/cpp11-migrate/LoopConvert/Inputs/structures.h

@@ -150,4 +150,27 @@ struct PtrSet {
   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

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*> 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.

clang-tools-extra/test/cpp11-migrate/LoopConvert/iterator_failing.cpp

@@ -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) {
-  }
-}