Use 'auto const&' for iterators whose deref operator return a const var

This patch fixes PR15601.
- Added check for whether the loop variable and the initializer have the same
type.
- Added tests.

llvm-svn: 181528

5 files changed, 97 insertions, 13 deletions

diff --git a/clang-tools-extra/cpp11-migrate/AddOverride/AddOverrideActions.cpp b/clang-tools-extra/cpp11-migrate/AddOverride/AddOverrideActions.cpp
index a40f673132c..3d1f1ff8c13 100644
--- a/clang-tools-extra/cpp11-migrate/AddOverride/AddOverrideActions.cpp
+++ b/clang-tools-extra/cpp11-migrate/AddOverride/AddOverrideActions.cpp
@@ -32,6 +32,10 @@ void AddOverrideFixer::run(const MatchFinder::MatchResult &Result) {
   const CXXMethodDecl *M = Result.Nodes.getDeclAs<CXXMethodDecl>(MethodId);
   assert(M && "Bad Callback. No node provided");
 
+  // Check that the method declaration in the main file
+  if (!SM.isFromMainFile(M->getLocStart()))
+    return;
+
   // First check that there isn't already an override attribute.
   if (!M->hasAttr<OverrideAttr>()) {
     if (M->getLocStart().isFileID()) {
diff --git a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.cpp b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.cpp
index 4ba22b3f284..88c219aefec 100644
--- a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.cpp
+++ b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.cpp
@@ -791,7 +791,8 @@ void LoopFixer::doConversion(ASTContext *Context,
                              bool AliasFromForInit,
                              const ForStmt *TheLoop,
                              bool ContainerNeedsDereference,
-                             bool DerefByValue) {
+                             bool DerefByValue,
+                             bool DerefByConstRef) {
   std::string VarName;
   bool VarNameFromAlias = Usages.size() == 1 && AliasDecl;
   bool AliasVarIsRef = false;
@@ -849,8 +850,11 @@ void LoopFixer::doConversion(ASTContext *Context,
     // to 'T&&'.
     if (DerefByValue)
       AutoRefType = Context->getRValueReferenceType(AutoRefType);
-    else
+    else {
+      if (DerefByConstRef)
+        AutoRefType = Context->getConstType(AutoRefType);
       AutoRefType = Context->getLValueReferenceType(AutoRefType);
+    }
   }
 
   std::string MaybeDereference = ContainerNeedsDereference ? "*" : "";
@@ -979,6 +983,7 @@ void LoopFixer::findAndVerifyUsages(ASTContext *Context,
                                     const Expr *BoundExpr,
                                     bool ContainerNeedsDereference,
                                     bool DerefByValue,
+                                    bool DerefByConstRef,
                                     const ForStmt *TheLoop,
                                     Confidence ConfidenceLevel) {
   ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
@@ -1013,7 +1018,7 @@ void LoopFixer::findAndVerifyUsages(ASTContext *Context,
   doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr),
                ContainerString, Finder.getUsages(), Finder.getAliasDecl(),
                Finder.aliasUseRequired(), Finder.aliasFromForInit(), TheLoop,
-               ContainerNeedsDereference, DerefByValue);
+               ContainerNeedsDereference, DerefByValue, DerefByConstRef);
   ++*AcceptedChanges;
 }
 
@@ -1051,14 +1056,67 @@ void LoopFixer::run(const MatchFinder::MatchResult &Result) {
     ConfidenceLevel.lowerTo(RL_Reasonable);
 
   const Expr *ContainerExpr = NULL;
+  bool DerefByValue = false;
+  bool DerefByConstRef = false;
   bool ContainerNeedsDereference = false;
   // FIXME: Try to put most of this logic inside a matcher. Currently, matchers
   // don't allow the right-recursive checks in digThroughConstructors.
-  if (FixerKind == LFK_Iterator)
+  if (FixerKind == LFK_Iterator) {
     ContainerExpr = findContainer(Context, LoopVar->getInit(),
                                   EndVar ? EndVar->getInit() : EndCall,
                                   &ContainerNeedsDereference);
-  else if (FixerKind == LFK_PseudoArray) {
+
+    QualType InitVarType = InitVar->getType();
+    QualType CanonicalInitVarType = InitVarType.getCanonicalType();
+
+    const CXXMemberCallExpr *BeginCall =
+        Nodes.getNodeAs<CXXMemberCallExpr>(BeginCallName);
+    assert(BeginCall != 0 && "Bad Callback. No begin call expression.");
+    QualType CanonicalBeginType =
+        BeginCall->getMethodDecl()->getResultType().getCanonicalType();
+
+    if (CanonicalBeginType->isPointerType() &&
+        CanonicalInitVarType->isPointerType()) {
+      QualType BeginPointeeType = CanonicalBeginType->getPointeeType();
+      QualType InitPointeeType = CanonicalInitVarType->getPointeeType();
+      // If the initializer and the variable are both pointers check if the
+      // un-qualified pointee types match otherwise we don't use auto.
+      if (!Context->hasSameUnqualifiedType(InitPointeeType, BeginPointeeType))
+        return;
+    } else {
+      // Check for qualified types to avoid conversions from non-const to const
+      // iterator types.
+      if (!Context->hasSameType(CanonicalInitVarType, CanonicalBeginType))
+        return;
+    }
+
+    DerefByValue = Nodes.getNodeAs<QualType>(DerefByValueResultName) != 0;
+    if (!DerefByValue) {
+      if (const QualType *DerefType =
+              Nodes.getNodeAs<QualType>(DerefByRefResultName)) {
+        // A node will only be bound with DerefByRefResultName if we're dealing
+        // with a user-defined iterator type. Test the const qualification of
+        // the reference type.
+        DerefByConstRef = (*DerefType)->getAs<ReferenceType>()->getPointeeType()
+            .isConstQualified();
+      } else {
+        // By nature of the matcher this case is triggered only for built-in
+        // iterator types (i.e. pointers).
+        assert(isa<PointerType>(CanonicalInitVarType) &&
+               "Non-class iterator type is not a pointer type");
+        QualType InitPointeeType = CanonicalInitVarType->getPointeeType();
+        QualType BeginPointeeType = CanonicalBeginType->getPointeeType();
+        // If the initializer and variable have both the same type just use auto
+        // otherwise we test for const qualification of the pointed-at type.
+        if (!Context->hasSameType(InitPointeeType, BeginPointeeType))
+          DerefByConstRef = InitPointeeType.isConstQualified();
+      }
+    } else {
+      // If the de-referece operator return by value then test for the canonical
+      // const qualification of the init variable type.
+      DerefByConstRef = CanonicalInitVarType.isConstQualified();
+    }
+  } else if (FixerKind == LFK_PseudoArray) {
     if (!EndCall)
       return;
     ContainerExpr = EndCall->getImplicitObjectArgument();
@@ -1071,9 +1129,7 @@ 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, DerefByValue, TheLoop,
-                      ConfidenceLevel);
+                      ContainerNeedsDereference, DerefByValue, DerefByConstRef,
+                      TheLoop, ConfidenceLevel);
 }
diff --git a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.h b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.h
index 6a488894ad3..eebee338208 100644
--- a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.h
+++ b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.h
@@ -77,7 +77,8 @@ class LoopFixer : public clang::ast_matchers::MatchFinder::MatchCallback {
                     bool AliasFromForInit,
                     const clang::ForStmt *TheLoop,
                     bool ContainerNeedsDereference,
-                    bool DerefByValue);
+                    bool DerefByValue,
+                    bool DerefByConstRef);
 
   /// \brief Given a loop header that would be convertible, discover all usages
   /// of the index variable and convert the loop if possible.
@@ -88,6 +89,7 @@ class LoopFixer : public clang::ast_matchers::MatchFinder::MatchCallback {
                            const clang::Expr *BoundExpr,
                            bool ContainerNeedsDereference,
                            bool DerefByValue,
+                           bool DerefByConstRef,
                            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 ffd9f794b0c..a7ca15f3eb3 100644
--- a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.cpp
+++ b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.cpp
@@ -22,10 +22,12 @@ const char ConditionBoundName[] = "conditionBound";
 const char ConditionVarName[] = "conditionVar";
 const char IncrementVarName[] = "incrementVar";
 const char InitVarName[] = "initVar";
+const char BeginCallName[] = "beginCall";
 const char EndCallName[] = "endCall";
 const char ConditionEndVarName[] = "conditionEndVar";
 const char EndVarName[] = "endVar";
 const char DerefByValueResultName[] = "derefByValueResult";
+const char DerefByRefResultName[] = "derefByRefResult";
 
 // shared matchers
 static const TypeMatcher AnyType = anything();
@@ -109,10 +111,23 @@ StatementMatcher makeArrayLoopMatcher() {
 ///   - If the end iterator variable 'g' is defined, it is the same as 'f'
 StatementMatcher makeIteratorLoopMatcher() {
   StatementMatcher BeginCallMatcher =
-      memberCallExpr(argumentCountIs(0), callee(methodDecl(hasName("begin"))));
+      memberCallExpr(
+        argumentCountIs(0),
+        callee(
+          methodDecl(hasName("begin"))
+        )
+      ).bind(BeginCallName);
 
   DeclarationMatcher InitDeclMatcher =
-      varDecl(hasInitializer(anything())).bind(InitVarName);
+      varDecl(
+        hasInitializer(
+          anyOf(
+            ignoringParenImpCasts(BeginCallMatcher),
+            materializeTemporaryExpr(ignoringParenImpCasts(BeginCallMatcher)),
+            hasDescendant(BeginCallMatcher)
+          )
+        )
+      ).bind(InitVarName);
 
   DeclarationMatcher EndDeclMatcher =
       varDecl(hasInitializer(anything())).bind(EndVarName);
@@ -157,7 +172,11 @@ StatementMatcher makeIteratorLoopMatcher() {
                 returns(
                   // Skip loops where the iterator's operator* returns an
                   // rvalue reference. This is just weird.
-                  qualType(unless(hasCanonicalType(rValueReferenceType())))
+                  qualType(
+                    unless(
+                      hasCanonicalType(rValueReferenceType())
+                    )
+                  ).bind(DerefByRefResultName)
                 )
               )
             )
@@ -165,6 +184,7 @@ StatementMatcher makeIteratorLoopMatcher() {
         )
       );
 
+
   return
     forStmt(
       hasLoopInit(anyOf(
diff --git a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.h b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.h
index 7824a82ad48..6946f49504b 100644
--- a/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.h
+++ b/clang-tools-extra/cpp11-migrate/LoopConvert/LoopMatchers.h
@@ -27,10 +27,12 @@ extern const char ConditionVarName[];
 extern const char ConditionEndVarName[];
 extern const char IncrementVarName[];
 extern const char InitVarName[];
+extern const char BeginCallName[];
 extern const char EndExprName[];
 extern const char EndCallName[];
 extern const char EndVarName[];
 extern const char DerefByValueResultName[];
+extern const char DerefByRefResultName[];
 
 clang::ast_matchers::StatementMatcher makeArrayLoopMatcher();
 clang::ast_matchers::StatementMatcher makeIteratorLoopMatcher();