Handle correctly containers that are data members in modernize-loop-convert.

[angelgarcia]

Summary

I recently found that the variable naming wasn't working as expected with containers that are data members. The new index always received the name "Elem" (or equivalent) regardless of the container's name.
The check was assuming that the container's declaration was a VarDecl, which cannot be converted to a FieldDecl (a data member), and then it could never retrieve its name.

This also fixes some cases where the check failed to find the container at all (so it didn't do any fix) because of the same reason.

[klimek]

lg

[angelgarcia]

Merge with the latest revision.

clang-tidy/modernize/LoopConvertCheck.h

struct RangeDescriptor {
     bool DerefByValue;
     std::string ContainerString;
     QualType ElemType;
   };
 
   void getAliasRange(SourceManager &SM, SourceRange &DeclRange);
 
   void doConversion(ASTContext *Context, const VarDecl *IndexVar,
-                    const VarDecl *MaybeContainer, const UsageResult &Usages,
+                    const ValueDecl *MaybeContainer, const UsageResult &Usages,
                     const DeclStmt *AliasDecl, bool AliasUseRequired,
                     bool AliasFromForInit, const ForStmt *Loop,
                     RangeDescriptor Descriptor);
 
   StringRef getContainerString(ASTContext *Context, const ForStmt *Loop,
                                const Expr *ContainerExpr);
 
   void getArrayLoopQualifiers(ASTContext *Context,

clang-tidy/modernize/LoopConvertCheck.cpp

if (SourceMgr.getFileID(Range.getBegin()) !=
     return StringRef(); // Empty string.
   }
 
   return Lexer::getSourceText(CharSourceRange(Range, true), SourceMgr,
                               LangOpts);
 }
 
 /// \brief If the given expression is actually a DeclRefExpr, find and return
-/// the underlying VarDecl; otherwise, return NULL.
-static const VarDecl *getReferencedVariable(const Expr *E) {
+/// the underlying ValueDecl; otherwise, return NULL.
+static const ValueDecl *getReferencedVariable(const Expr *E) {
   if (const DeclRefExpr *DRE = getDeclRef(E))
     return dyn_cast<VarDecl>(DRE->getDecl());
+  if (const auto *Mem = dyn_cast<MemberExpr>(E))
+    return dyn_cast<FieldDecl>(Mem->getMemberDecl());
   return nullptr;
 }
 
 /// \brief Returns true when the given expression is a member expression
 /// whose base is `this` (implicitly or not).
 static bool isDirectMemberExpr(const Expr *E) {
   if (const auto *Member = dyn_cast<MemberExpr>(E->IgnoreParenImpCasts()))
     return isa<CXXThisExpr>(Member->getBase()->IgnoreParenImpCasts());
void LoopConvertCheck::getAliasRange(SourceManager &SM, SourceRange &Range) {
   unsigned Offset = std::strspn(TextAfter, " \t\r\n");
   Range =
       SourceRange(Range.getBegin(), Range.getEnd().getLocWithOffset(Offset));
 }
 
 /// \brief Computes the changes needed to convert a given for loop, and
 /// applies them.
 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) {
+    ASTContext *Context, const VarDecl *IndexVar,
+    const ValueDecl *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) {

clang-tidy/modernize/LoopConvertUtils.h

enum NamingStyle {
     NS_CamelCase,
     NS_LowerCase,
     NS_UpperCase,
   };
 
   VariableNamer(StmtGeneratedVarNameMap *GeneratedDecls,
                 const StmtParentMap *ReverseAST, const clang::Stmt *SourceStmt,
                 const clang::VarDecl *OldIndex,
-                const clang::VarDecl *TheContainer,
+                const clang::ValueDecl *TheContainer,
                 const clang::ASTContext *Context, NamingStyle Style)
       : GeneratedDecls(GeneratedDecls), ReverseAST(ReverseAST),
         SourceStmt(SourceStmt), OldIndex(OldIndex), TheContainer(TheContainer),
         Context(Context), Style(Style) {}
 
   /// \brief Generate a new index name.
   ///
   /// Generates the name to be used for an inserted iterator. It relies on
   /// declarationExists() to determine that there are no naming conflicts, and
   /// tries to use some hints from the container name and the old index name.
   std::string createIndexName();
 
 private:
   StmtGeneratedVarNameMap *GeneratedDecls;
   const StmtParentMap *ReverseAST;
   const clang::Stmt *SourceStmt;
   const clang::VarDecl *OldIndex;
-  const clang::VarDecl *TheContainer;
+  const clang::ValueDecl *TheContainer;
   const clang::ASTContext *Context;
   const NamingStyle Style;
 
   // Determine whether or not a declaration that would conflict with Symbol
   // exists in an outer context or in any statement contained in SourceStmt.
   bool declarationExists(llvm::StringRef Symbol);
 
   // Concatenates two identifiers following the current naming style.
   std::string AppendWithStyle(StringRef Str, StringRef Suffix) const;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_UTILS_H

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

for (int I = 0; I < N; ++I) {
       printf("%d", this->ValArr[I].X);
     }
     // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead
     // CHECK-FIXES: for (auto & Elem : this->ValArr)
     // CHECK-FIXES-NEXT: printf("%d", Elem.X);
   }
 };
 
+struct HasIndirectArr {
+  HasArr HA;
+  void implicitThis() {
+    for (int I = 0; I < N; ++I) {
+      printf("%d", HA.Arr[I]);
+    }
+    // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead
+    // CHECK-FIXES: for (int Elem : HA.Arr)
+    // CHECK-FIXES-NEXT: printf("%d", Elem);
+
+    for (int I = 0; I < N; ++I) {
+      printf("%d", HA.ValArr[I].X);
+    }
+    // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead
+    // CHECK-FIXES: for (auto & Elem : HA.ValArr)
+    // CHECK-FIXES-NEXT: printf("%d", Elem.X);
+  }
+
+  void explicitThis() {
+    for (int I = 0; I < N; ++I) {
+      printf("%d", this->HA.Arr[I]);
+    }
+    // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead
+    // CHECK-FIXES: for (int Elem : this->HA.Arr)
+    // CHECK-FIXES-NEXT: printf("%d", Elem);
+
+    for (int I = 0; I < N; ++I) {
+      printf("%d", this->HA.ValArr[I].X);
+    }
+    // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead
+    // CHECK-FIXES: for (auto & Elem : this->HA.ValArr)
+    // CHECK-FIXES-NEXT: printf("%d", Elem.X);
+  }
+};
+
 // Loops whose bounds are value-dependent should not be converted.
 template <int N>
 void dependentExprBound() {
   for (int I = 0; I < N; ++I)
     Arr[I] = 0;
 }
 template void dependentExprBound<20>();
 

test/clang-tidy/modernize-loop-convert-const.cpp

class TestInsideConstFunction {
   vector<int> V;
 
   void foo() const {
     for (int I = 0; I < N; ++I) {
       if (Ints[I])
         copyArg(Ints[I]);
     }
     // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop
-    // CHECK-FIXES: for (int Elem : Ints)
+    // CHECK-FIXES: for (int Int : Ints)
 
     for (int I = 0; I < N; ++I) {
       Array[I].constMember(0);
       constRefArg(Array[I]);
     }
     // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop
     // CHECK-FIXES: for (auto Elem : Array)
 

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

void refs_and_vals() {
   // CHECK-FIXES-NEXT: const int& Idx = Other[0];
   // CHECK-FIXES-NEXT: unsigned Othersize = Other.size();
 
   for (int I = 0, E = Dep.size(); I != E; ++I) {
     int Idx = Other.at(I);
   }
 }
 
+struct MemberNaming {
+  const static int N = 10;
+  int Ints[N], Ints_[N];
+  void loops() {
+    for (int I = 0; I < N; ++I) {
+      printf("%d\n", Ints[I]);
+    }
+    // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead
+    // CHECK-FIXES: for (int Int : Ints)
+    // CHECK-FIXES-NEXT: printf("%d\n", Int);
+
+    for (int I = 0; I < N; ++I) {
+      printf("%d\n", Ints_[I]);
+    }
+    // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead
+    // CHECK-FIXES: for (int Int : Ints_)
+    // CHECK-FIXES-NEXT: printf("%d\n", Int);
+  }
+};
+
 } // namespace NamingAlias
 
 namespace NamingConlict {
 
 #define MAX(a, b) (a > b) ? a : b
 #define DEF 5
 
 const int N = 10;

test/clang-tidy/modernize-loop-convert-negative.cpp

for (int I = 0; I < N; ++I)
     Sum += Arr[I] + BadArr[I];
 
   for (int I = 0; I < N; ++I) {
     int K = BadArr[I];
     Sum += Arr[I] + K;
   }
 }
 
-struct HasArr {
-  int Arr[N];
-  Val ValArr[N];
-};
-
-struct HasIndirectArr {
-  HasArr HA;
-  void implicitThis() {
-    for (int I = 0; I < N; ++I) {
-      printf("%d", HA.Arr[I]);
-    }
-
-    for (int I = 0; I < N; ++I) {
-      printf("%d", HA.ValArr[I].X);
-    }
-  }
-
-  void explicitThis() {
-    for (int I = 0; I < N; ++I) {
-      printf("%d", this->HA.Arr[I]);
-    }
-
-    for (int I = 0; I < N; ++I) {
-      printf("%d", this->HA.ValArr[I].X);
-    }
-  }
-};
 }
 
 namespace NegativeIterator {
 
 S Ss;
 T Tt;
 U Tu;