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

[angelgarcia]

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.

[klimek]

lg

clang-tidy/modernize/LoopConvertCheck.cpp

529 ↗

(On Diff #36497)

Use "UOP".

[angelgarcia]

Use "UOP".

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

void LoopConvertCheck::doConversion(
     ASTContext *Context, const VarDecl *IndexVar, const VarDecl *MaybeContainer,
     const UsageResult &Usages, const DeclStmt *AliasDecl, bool AliasUseRequired,
     bool AliasFromForInit, const ForStmt *Loop, RangeDescriptor Descriptor) {
   auto Diag = diag(Loop->getForLoc(), "use range-based for loop instead");
 
   std::string VarName;
   bool VarNameFromAlias = (Usages.size() == 1) && AliasDecl;
   bool AliasVarIsRef = false;
+  bool CanCopy = true;
 
   if (VarNameFromAlias) {
     const auto *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.
     SourceRange ReplaceRange = AliasDecl->getSourceRange();
for (const auto &Usage : Usages) {
                                                                  : VarName;
         auto Parents = Context->getParents(*Usage.Expression);
         if (Parents.size() == 1) {
           if (const auto *Paren = Parents[0].get<ParenExpr>()) {
             // Usage.Expression will be replaced with the new index variable,
             // 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 {
         // The Usage expression is only null in case of lambda captures (which
         // are VarDecl). If the index is captured by value, add '&' to capture
         // by reference instead.
         ReplaceText =
             Usage.Kind == Usage::UK_CaptureByCopy ? "&" + VarName : VarName;
       }
       TUInfo->getReplacedVars().insert(std::make_pair(Loop, IndexVar));
       Diag << FixItHint::CreateReplacement(
           CharSourceRange::getTokenRange(Range), ReplaceText);
     }
   }
 
   // Now, we need to construct the new range expression.
   SourceRange ParenRange(Loop->getLParenLoc(), Loop->getRParenLoc());
 
   QualType AutoType = 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.
-  bool UseCopy = (VarNameFromAlias && !AliasVarIsRef) ||
-                 (Descriptor.DerefByConstRef && Descriptor.IsTriviallyCopyable);
+  bool UseCopy =
+      CanCopy &&
+      ((VarNameFromAlias && !AliasVarIsRef) ||
+       (Descriptor.DerefByConstRef && Descriptor.IsTriviallyCopyable));
 
   if (!UseCopy) {
     if (Descriptor.DerefByConstRef) {
       AutoType =
           Context->getLValueReferenceType(Context->getConstType(AutoType));
     } else if (Descriptor.DerefByValue) {
       if (!Descriptor.IsTriviallyCopyable)
         AutoType = Context->getRValueReferenceType(AutoType);

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

void f() {
   for (int I = 0; I < N; ++I) {
     Teas[I].g();
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & Tea : Teas)
   // 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]);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto Elem : ConstArr)
   // CHECK-FIXES-NEXT: printf("2 * %d = %d\n", Elem, Elem + Elem);
 
   const NonTriviallyCopyable NonCopy[N]{};
   for (int I = 0; I < N; ++I) {
     printf("2 * %d = %d\n", NonCopy[I].X, NonCopy[I].X + NonCopy[I].X);
   }
   // 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 {
   int Arr[N];
   Val ValArr[N];
   void implicitThis() {
     for (int I = 0; I < N; ++I) {
       printf("%d", Arr[I]);