[PATCH] Improve loop convert's variable aliasing

[Jack Yang]

Hi revane, gribozavr,

Loop convert's variable name aliasing checks for whether a variable
is declared inside the loop body to store each element of the container.
If found, the loop variable is declared as a reference with the same name.
This may cause problems since the variable declaration may be by value.
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 vice versa for
references.

Fixes: PR15600

http://llvm-reviews.chandlerc.com/D588

Files:
  cpp11-migrate/LoopConvert/LoopActions.cpp
  test/cpp11-migrate/LoopConvert/naming-alias.cpp

Index: cpp11-migrate/LoopConvert/LoopActions.cpp
===================================================================
--- cpp11-migrate/LoopConvert/LoopActions.cpp
+++ cpp11-migrate/LoopConvert/LoopActions.cpp
@@ -737,10 +737,14 @@
                              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,13 +774,18 @@
 
   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);
+  // 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 || (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&&'.
+    if (DerefByValue)
+      AutoRefType = Context->getRValueReferenceType(AutoRefType);
+    else
+      AutoRefType = Context->getLValueReferenceType(AutoRefType);
+  }
 
   std::string MaybeDereference = ContainerNeedsDereference ? "*" : "";
   std::string TypeString = AutoRefType.getAsString();
Index: test/cpp11-migrate/LoopConvert/naming-alias.cpp
===================================================================
--- test/cpp11-migrate/LoopConvert/naming-alias.cpp
+++ test/cpp11-migrate/LoopConvert/naming-alias.cpp
@@ -57,3 +57,40 @@
   // 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;
+}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D588.1.patch
Type: text/x-patch
Size: 3466 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130328/af841adb/attachment.bin>