Document a bug in loop-convert and fix one of its subcases.

Summary: Now that we prioritize copying trivial types over using const-references where possible, I found some cases where, after the transformation, the loop was using the address of the local copy instead of the original object.

Reviewers: klimek

Subscribers: alexfh, cfe-commits

Differential Revision: http://reviews.llvm.org/D13431

llvm-svn: 249300

Author: angargo

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

@@ -477,6 +477,7 @@ void LoopConvertCheck::doConversion(
   std::string VarName;
   bool VarNameFromAlias = (Usages.size() == 1) && AliasDecl;
   bool AliasVarIsRef = false;
+  bool CanCopy = true;
 
   if (VarNameFromAlias) {
     const auto *AliasVar = cast<VarDecl>(AliasDecl->getSingleDecl());
@@ -525,6 +526,16 @@ void LoopConvertCheck::doConversion(
             // and parenthesis around a simple DeclRefExpr can always be
             // removed.
             Range = Paren->getSourceRange();
+          } else if (const auto *UOP = Parents[0].get<UnaryOperator>()) {
+            // If we are taking the address of the loop variable, then we must
+            // not use a copy, as it would mean taking the address of the loop's
+            // local index instead.
+            // FIXME: This won't catch cases where the address is taken outside
+            // of the loop's body (for instance, in a function that got the
+            // loop's index as a const reference parameter), or where we take
+            // the address of a member (like "&Arr[i].A.B.C").
+            if (UOP->getOpcode() == UO_AddrOf)
+              CanCopy = false;
           }
         }
       } else {
@@ -548,8 +559,10 @@ void LoopConvertCheck::doConversion(
   // 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.
-  bool UseCopy = (VarNameFromAlias && !AliasVarIsRef) ||
-                 (Descriptor.DerefByConstRef && Descriptor.IsTriviallyCopyable);
+  bool UseCopy =
+      CanCopy &&
+      ((VarNameFromAlias && !AliasVarIsRef) ||
+       (Descriptor.DerefByConstRef && Descriptor.IsTriviallyCopyable));
 
   if (!UseCopy) {
     if (Descriptor.DerefByConstRef) {

clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp

@@ -97,7 +97,7 @@ void f() {
   // CHECK-FIXES-NEXT: Tea.g();
 }
 
-void constArray() {
+const int *constArray() {
   for (int I = 0; I < N; ++I) {
     printf("2 * %d = %d\n", ConstArr[I], ConstArr[I] + ConstArr[I]);
   }
@@ -112,6 +112,16 @@ void constArray() {
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (const auto & Elem : NonCopy)
   // CHECK-FIXES-NEXT: printf("2 * %d = %d\n", Elem.X, Elem.X + Elem.X);
+
+  bool Something = false;
+  for (int I = 0; I < N; ++I) {
+    if (Something)
+      return &ConstArr[I];
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (const auto & Elem : ConstArr)
+  // CHECK-FIXES-NEXT: if (Something)
+  // CHECK-FIXES-NEXT: return &Elem;
 }
 
 struct HasArr {