[PATCH] D13431: Document a bug in loop-convert and fix one of its subcases.

[Angel Garcia via cfe-commits]

angelgarcia created this revision.
angelgarcia added a reviewer: klimek.
angelgarcia added subscribers: cfe-commits, alexfh.

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.

http://reviews.llvm.org/D13431

Files:
  clang-tidy/modernize/LoopConvertCheck.cpp
  test/clang-tidy/modernize-loop-convert-basic.cpp

Index: test/clang-tidy/modernize-loop-convert-basic.cpp
===================================================================
--- test/clang-tidy/modernize-loop-convert-basic.cpp
+++ test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -97,7 +97,7 @@
   // 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 @@
   // 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 {
Index: clang-tidy/modernize/LoopConvertCheck.cpp
===================================================================
--- clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tidy/modernize/LoopConvertCheck.cpp
@@ -477,6 +477,7 @@
   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 @@
             // 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 @@
   // 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) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D13431.36497.patch
Type: text/x-patch
Size: 2918 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151005/ac75ed5f/attachment-0001.bin>