[PATCH] Improve loop convert's variable aliasing

[Jack Yang]

  - Simplified if statement as per Edwin's comment

  And yes, if a loop uses an "alias" variable as well as the iterator/array it won't be marked as an aliased variable.
  In the statement:

    bool VarNameFromAlias = Usages.size() == 1 && AliasDecl;

  Usages.size() returns how many times the iterator or array was called.
  AliasDecl is populated by isAliasDecl() which finds the aliased variable declaration inside the loop body.

  If both statements are true, then we know that the iterator/array was only used once in the aliased variable declaration.

Hi revane, gribozavr,

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

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D588?vs=1432&id=1459#toc

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 || 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.2.patch
Type: text/x-patch
Size: 3444 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130401/3d99fa32/attachment.bin>