Improve loop convert's variable aliasing

Loop convert's variable name aliasing may cause issues if the variable is
declared as a value (copy). The converted loop will declare the variable as a
reference which may inadvertently cause modifications to the container if it
were used and modified as a temporary copy.

This is fixed by preserving the reference or value qualifiers of the aliased
variable. That is, if the variable was declared as a value the loop variable
will also be declared as a value and similarly for references.

Fixes: PR15600
Author: Jack Yang <jack.yang@intel.com>
llvm-svn: 178485

clang-tools-extra/cpp11-migrate/LoopConvert/LoopActions.cpp

@@ -737,10 +737,14 @@ void LoopFixer::doConversion(ASTContext *Context,
                              bool ContainerNeedsDereference,
                              bool DerefByValue) {
   std::string VarName;
+  bool VarNameFromAlias = Usages.size() == 1 && AliasDecl;
+  bool AliasVarIsRef = false;
 
-  if (Usages.size() == 1 && AliasDecl) {
+  if (VarNameFromAlias) {
     const VarDecl *AliasVar = cast<VarDecl>(AliasDecl->getSingleDecl());
     VarName = AliasVar->getName().str();
+    AliasVarIsRef = AliasVar->getType()->isReferenceType();
+
     // We keep along the entire DeclStmt to keep the correct range here.
     const SourceRange &ReplaceRange = AliasDecl->getSourceRange();
     Replace->insert(
@@ -770,6 +774,10 @@ void LoopFixer::doConversion(ASTContext *Context,
 
   QualType AutoRefType = Context->getAutoDeductType();
 
+  // If the new variable name is from the aliased variable, then the reference
+  // type for the new variable should only be used if the aliased variable was
+  // declared as a reference.
+  if (!VarNameFromAlias || AliasVarIsRef) {
     // 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&&'.
@@ -777,6 +785,7 @@ void LoopFixer::doConversion(ASTContext *Context,
       AutoRefType = Context->getRValueReferenceType(AutoRefType);
     else
       AutoRefType = Context->getLValueReferenceType(AutoRefType);
+  }
 
   std::string MaybeDereference = ContainerNeedsDereference ? "*" : "";
   std::string TypeString = AutoRefType.getAsString();

clang-tools-extra/test/cpp11-migrate/LoopConvert/naming-alias.cpp

@@ -57,3 +57,40 @@ void aliasing() {
   // CHECK-NEXT: Val &t = func(elem);
   // CHECK-NEXT: int y = t.x;
 }
+
+void refs_and_vals() {
+  // The following tests check that the transform correctly preserves the
+  // reference or value qualifiers of the aliased variable. That is, if the
+  // variable was declared as a value, the loop variable will be declared as a
+  // value and vice versa for references.
+
+  S s;
+  const S s_const = s;
+
+  for (S::const_iterator it = s_const.begin(); it != s_const.end(); ++it) {
+    MutableVal alias = *it; { }
+    alias.x = 0;
+  }
+  // CHECK: for (auto alias : s_const)
+  // CHECK-NOT: MutableVal {{[a-z_]+}} =
+  // CHECK-NEXT: { }
+  // CHECK-NEXT: alias.x = 0;
+
+  for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
+    MutableVal alias = *it; { }
+    alias.x = 0;
+  }
+  // CHECK: for (auto alias : s)
+  // CHECK-NOT: MutableVal {{[a-z_]+}} =
+  // CHECK-NEXT: { }
+  // CHECK-NEXT: alias.x = 0;
+
+  for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
+    MutableVal &alias = *it; { }
+    alias.x = 0;
+  }
+  // CHECK: for (auto & alias : s)
+  // CHECK-NOT: MutableVal &{{[a-z_]+}} =
+  // CHECK-NEXT: { }
+  // CHECK-NEXT: alias.x = 0;
+}