Refactor LoopConvertCheck.

[angelgarcia]

Summary

Reorder the code in a more logical and understandable way.

[klimek]

clang-tidy/modernize/LoopConvertCheck.cpp

419

Now you can make that early exit!

494–495

I assume the change in the tests is due to the bug here? :)

498

I'd give the intermediate result a different name.

533–534

I think we still want to split up this function. The if() {} else {} blocks are too long ...

534

This sentence doesn't parse.

534–536

I think an example might help this comment. Here and elsewhere ;)

535–536

Perhaps add: , because we need to keep the code correct if it mutates the returned objects

536

LLVM uses FIXME; also, "Think about this" is not a good todo ;) They need to be more specific (or delete it).

542

That comment doesn't give information, I think. I'd delete it.

559–560

I liked the early continue better...

clang-tidy/modernize/LoopConvertCheck.h

34

I'd probably make that a std::string - having references in members is a frequent cause for bugs.

[angelgarcia]

clang-tidy/modernize/LoopConvertCheck.cpp

494–495

I'm not sure if it was a bug or just different priorities, but that's not reason. "DerefByValue" is a weird case and we don't have many tests for that. The change is due to:

If the initializer and variable have both the same type just use auto
otherwise we test for const qualification of the pointed-at type.

At line 726 (somewhere inside the iterator loops logic). I removed the conditional because we always want to check for const qualification.

[angelgarcia]

Split a function and several other small changes.

[klimek]

lg

clang-tidy/modernize/LoopConvertCheck.h

 public:
   LoopConvertCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   struct RangeDescriptor {
+    RangeDescriptor();
     bool ContainerNeedsDereference;
     bool DerefByConstRef;
     bool DerefByValue;
     bool IsTriviallyCopyable;
+    std::string ContainerString;
   };
 
   void doConversion(ASTContext *Context, const VarDecl *IndexVar,
-                    const VarDecl *MaybeContainer, StringRef ContainerString,
-                    const UsageResult &Usages, const DeclStmt *AliasDecl,
-                    bool AliasUseRequired, bool AliasFromForInit,
-                    const ForStmt *TheLoop, RangeDescriptor Descriptor);
-
-  StringRef checkRejections(ASTContext *Context, const Expr *ContainerExpr,
-                            const ForStmt *TheLoop);
-
-  void findAndVerifyUsages(ASTContext *Context, const VarDecl *LoopVar,
-                           const VarDecl *EndVar, const Expr *ContainerExpr,
-                           const Expr *BoundExpr, const ForStmt *TheLoop,
-                           LoopFixerKind FixerKind, RangeDescriptor Descriptor);
+                    const VarDecl *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,
+                              const ast_matchers::BoundNodes &Nodes,
+                              const Expr *ContainerExpr,
+                              const UsageResult &Usages,
+                              RangeDescriptor &Descriptor);
+
+  void getIteratorLoopQualifiers(ASTContext *Context,
+                                 const ast_matchers::BoundNodes &Nodes,
+                                 RangeDescriptor &Descriptor);
+
+  void determineRangeDescriptor(ASTContext *Context,
+                                const ast_matchers::BoundNodes &Nodes,
+                                const ForStmt *Loop, LoopFixerKind FixerKind,
+                                const Expr *ContainerExpr,
+                                const UsageResult &Usages,
+                                RangeDescriptor &Descriptor);
+
+  bool isConvertible(ASTContext *Context, const ast_matchers::BoundNodes &Nodes,
+                     const ForStmt *Loop, LoopFixerKind FixerKind);
 
   std::unique_ptr<TUTrackingInfo> TUInfo;
   Confidence::Level MinConfidence;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H

clang-tidy/modernize/LoopConvertCheck.cpp

if (Dereference) {
         return false;
       CType = CType->getPointeeType();
     }
     return CType.isConstQualified();
   }
   return false;
 }
 
+LoopConvertCheck::RangeDescriptor::RangeDescriptor()
+    : ContainerNeedsDereference(false), DerefByConstRef(false),
+      DerefByValue(false), IsTriviallyCopyable(false) {}
+
 LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
       MinConfidence(StringSwitch<Confidence::Level>(
                         Options.get("MinConfidence", "reasonable"))
                         .Case("safe", Confidence::CL_Safe)
                         .Case("risky", Confidence::CL_Risky)
                         .Default(Confidence::CL_Reasonable)) {}
 
 void LoopConvertCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   SmallVector<std::string, 3> Confs{"risky", "reasonable", "safe"};
   Options.store(Opts, "MinConfidence", Confs[static_cast<int>(MinConfidence)]);
 }
 
+void LoopConvertCheck::registerMatchers(MatchFinder *Finder) {
+  // Only register the matchers for C++. Because this checker is used for
+  // modernization, it is reasonable to run it on any C++ standard with the
+  // assumption the user is trying to modernize their codebase.
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(makeArrayLoopMatcher(), this);
+  Finder->addMatcher(makeIteratorLoopMatcher(), this);
+  Finder->addMatcher(makePseudoArrayLoopMatcher(), this);
+}
+
 /// \brief Computes the changes needed to convert a given for loop, and
-/// applies it.
+/// applies them.
 void LoopConvertCheck::doConversion(
     ASTContext *Context, const VarDecl *IndexVar, const VarDecl *MaybeContainer,
-    StringRef ContainerString, const UsageResult &Usages,
-    const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit,
-    const ForStmt *TheLoop, RangeDescriptor Descriptor) {
-  // If there aren't any usages, converting the loop would generate an unused
-  // variable warning.
-  if (Usages.size() == 0)
-    return;
-
-  auto Diag = diag(TheLoop->getForLoc(), "use range-based for loop instead");
+    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;
 
   if (VarNameFromAlias) {
     const auto *AliasVar = cast<VarDecl>(AliasDecl->getSingleDecl());
     VarName = AliasVar->getName().str();
if (VarNameFromAlias) {
 
     Diag << FixItHint::CreateReplacement(
         CharSourceRange::getTokenRange(ReplaceRange), ReplacementText);
     // No further replacements are made to the loop, since the iterator or index
     // was used exactly once - in the initialization of AliasVar.
   } else {
     VariableNamer Namer(&TUInfo->getGeneratedDecls(),
                         &TUInfo->getParentFinder().getStmtToParentStmtMap(),
-                        TheLoop, IndexVar, MaybeContainer, Context);
+                        Loop, IndexVar, MaybeContainer, Context);
     VarName = Namer.createIndexName();
     // First, replace all usages of the array subscript expression with our new
     // variable.
     for (const auto &Usage : Usages) {
       std::string ReplaceText;
       if (Usage.Expression) {
         // If this is an access to a member through the arrow operator, after
         // the replacement it must be accessed through the '.' operator.
         ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow ? VarName + "."
                                                                  : VarName;
       } 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(TheLoop, IndexVar));
+      TUInfo->getReplacedVars().insert(std::make_pair(Loop, IndexVar));
       Diag << FixItHint::CreateReplacement(
           CharSourceRange::getTokenRange(Usage.Range), ReplaceText);
     }
   }
 
   // Now, we need to construct the new range expression.
-  SourceRange ParenRange(TheLoop->getLParenLoc(), TheLoop->getRParenLoc());
+  SourceRange ParenRange(Loop->getLParenLoc(), Loop->getRParenLoc());
 
-  QualType AutoRefType = Context->getAutoDeductType();
+  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.
   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 (Descriptor.DerefByValue) {
+    if (Descriptor.DerefByConstRef) {
+      AutoType =
+          Context->getLValueReferenceType(Context->getConstType(AutoType));
+    } else if (Descriptor.DerefByValue) {
       if (!Descriptor.IsTriviallyCopyable)
-        AutoRefType = Context->getRValueReferenceType(AutoRefType);
+        AutoType = Context->getRValueReferenceType(AutoType);
     } else {
-      if (Descriptor.DerefByConstRef)
-        AutoRefType = Context->getConstType(AutoRefType);
-      AutoRefType = Context->getLValueReferenceType(AutoRefType);
+      AutoType = Context->getLValueReferenceType(AutoType);
     }
   }
 
   StringRef MaybeDereference = Descriptor.ContainerNeedsDereference ? "*" : "";
-  std::string TypeString = AutoRefType.getAsString();
+  std::string TypeString = AutoType.getAsString();
   std::string Range = ("(" + TypeString + " " + VarName + " : " +
-                       MaybeDereference + ContainerString + ")")
+                       MaybeDereference + Descriptor.ContainerString + ")")
                           .str();
   Diag << FixItHint::CreateReplacement(
       CharSourceRange::getTokenRange(ParenRange), Range);
-  TUInfo->getGeneratedDecls().insert(make_pair(TheLoop, VarName));
+  TUInfo->getGeneratedDecls().insert(make_pair(Loop, VarName));
 }
 
-/// \brief Determine if the change should be deferred or rejected, returning
-/// text which refers to the container iterated over if the change should
-/// proceed.
-StringRef LoopConvertCheck::checkRejections(ASTContext *Context,
-                                            const Expr *ContainerExpr,
-                                            const ForStmt *TheLoop) {
-  // If we already modified the range of this for loop, don't do any further
-  // updates on this iteration.
-  if (TUInfo->getReplacedVars().count(TheLoop))
-    return "";
-
-  Context->getTranslationUnitDecl();
-  TUInfo->getParentFinder();
-  TUInfo->getParentFinder().gatherAncestors(Context->getTranslationUnitDecl());
-  // Ensure that we do not try to move an expression dependent on a local
-  // variable declared inside the loop outside of it.
-  DependencyFinderASTVisitor DependencyFinder(
-      &TUInfo->getParentFinder().getStmtToParentStmtMap(),
-      &TUInfo->getParentFinder().getDeclToParentStmtMap(),
-      &TUInfo->getReplacedVars(), TheLoop);
-
-  // FIXME: Determine when the external dependency isn't an expression converted
-  // by another loop.
-  if (DependencyFinder.dependsOnInsideVariable(ContainerExpr))
-    return "";
-
+/// \brief Returns a string which refers to the container iterated over.
+StringRef LoopConvertCheck::getContainerString(ASTContext *Context,
+                                               const ForStmt *Loop,
+                                               const Expr *ContainerExpr) {
   StringRef ContainerString;
   if (isa<CXXThisExpr>(ContainerExpr->IgnoreParenImpCasts())) {
     ContainerString = "this";
   } else {
     ContainerString =
         getStringFromRange(Context->getSourceManager(), Context->getLangOpts(),
                            ContainerExpr->getSourceRange());
   }
 
   return ContainerString;
 }
 
-/// \brief Given a loop header that would be convertible, discover all usages
-/// of the index variable and convert the loop if possible.
-void LoopConvertCheck::findAndVerifyUsages(
-    ASTContext *Context, const VarDecl *LoopVar, const VarDecl *EndVar,
-    const Expr *ContainerExpr, const Expr *BoundExpr, const ForStmt *TheLoop,
-    LoopFixerKind FixerKind, RangeDescriptor Descriptor) {
-  ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
-                                BoundExpr,
-                                Descriptor.ContainerNeedsDereference);
-
-  if (ContainerExpr) {
-    ComponentFinderASTVisitor ComponentFinder;
-    ComponentFinder.findExprComponents(ContainerExpr->IgnoreParenImpCasts());
-    Finder.addComponents(ComponentFinder.getComponents());
-  }
-
-  if (!Finder.findAndVerifyUsages(TheLoop->getBody()))
-    return;
-
-  Confidence ConfidenceLevel(Finder.getConfidenceLevel());
-  if (FixerKind == LFK_Array) {
-    // The array being indexed by IndexVar was discovered during traversal.
-    ContainerExpr = Finder.getContainerIndexed()->IgnoreParenImpCasts();
-
-    // Very few loops are over expressions that generate arrays rather than
-    // array variables. Consider loops over arrays that aren't just represented
-    // by a variable to be risky conversions.
-    if (!getReferencedVariable(ContainerExpr) &&
-        !isDirectMemberExpr(ContainerExpr))
-      ConfidenceLevel.lowerTo(Confidence::CL_Risky);
-
-    // Use 'const' if the array is const.
-    if (containerIsConst(ContainerExpr, Descriptor.ContainerNeedsDereference))
-      Descriptor.DerefByConstRef = true;
-
-  } else if (FixerKind == LFK_PseudoArray) {
-    if (!Descriptor.DerefByValue && !Descriptor.DerefByConstRef) {
-      const UsageResult &Usages = Finder.getUsages();
-      if (usagesAreConst(Usages) ||
-          containerIsConst(ContainerExpr,
-                           Descriptor.ContainerNeedsDereference)) {
-        Descriptor.DerefByConstRef = true;
-      } else if (usagesReturnRValues(Usages)) {
-        // If the index usages (dereference, subscript, at) return RValues,
-        // then we should not use a non-const reference.
-        Descriptor.DerefByValue = true;
-        // Try to find the type of the elements on the container from the
-        // usages.
-        for (const Usage &U : Usages) {
-          if (!U.Expression || U.Expression->getType().isNull())
-            continue;
-          QualType Type = U.Expression->getType().getCanonicalType();
-          if (U.Kind == Usage::UK_MemberThroughArrow) {
-            if (!Type->isPointerType())
-              continue;
-            Type = Type->getPointeeType();
-          }
-          Descriptor.IsTriviallyCopyable =
-              Type.isTriviallyCopyableType(*Context);
-        }
-      }
-    }
-  }
+/// \brief Determines what kind of 'auto' must be used after converting a for
+/// loop that iterates over an array or pseudoarray.
+void LoopConvertCheck::getArrayLoopQualifiers(ASTContext *Context,
+                                              const BoundNodes &Nodes,
+                                              const Expr *ContainerExpr,
+                                              const UsageResult &Usages,
+                                              RangeDescriptor &Descriptor) {
+  // On arrays and pseudoarrays, we must figure out the qualifiers from the
+  // usages.
+  if (usagesAreConst(Usages)) {
+  } else {
+    if (usagesReturnRValues(Usages)) {
+      // If the index usages (dereference, subscript, at, ...) return rvalues,
+      // then we should not use a reference, because we need to keep the code
+      // correct if it mutates the returned objects.
+      // Try to find the type of the elements on the container, to check if
+      // they are trivially copyable.
+        Descriptor.IsTriviallyCopyable = Type.isTriviallyCopyableType(*Context);
+        break;
+    } else if (containerIsConst(ContainerExpr,
+                                Descriptor.ContainerNeedsDereference)) {
+      // The usages are lvalue references, we add a 'const' if the container
+      // is 'const'. This will always be the case with const arrays (unless
+      // usagesAreConst() returned true).
+      Descriptor.DerefByConstRef = true;
 
-  StringRef ContainerString = checkRejections(Context, ContainerExpr, TheLoop);
+/// \brief Determines what kind of 'auto' must be used after converting an
+/// iterator based for loop.
+void LoopConvertCheck::getIteratorLoopQualifiers(ASTContext *Context,
+                                                 const BoundNodes &Nodes,
+                                                 RangeDescriptor &Descriptor) {
+  // The matchers for iterator loops provide bound nodes to obtain this
+  // information.
+  const auto *InitVar = Nodes.getDeclAs<VarDecl>(InitVarName);
+  QualType CanonicalInitVarType = InitVar->getType().getCanonicalType();
+  const auto *DerefByValueType =
+      Nodes.getNodeAs<QualType>(DerefByValueResultName);
+  Descriptor.DerefByValue = DerefByValueType;
 
-  if (ContainerString.empty() || ConfidenceLevel.getLevel() < MinConfidence)
-    return;
+  if (Descriptor.DerefByValue) {
+    // If the dereference operator returns by value then test for the
+    // canonical const qualification of the init variable type.
+    Descriptor.DerefByConstRef = CanonicalInitVarType.isConstQualified();
+    Descriptor.IsTriviallyCopyable =
+        DerefByValueType->isTriviallyCopyableType(*Context);
+  } else {
+    if (const auto *DerefType =
+            Nodes.getNodeAs<QualType>(DerefByRefResultName)) {
+      // A node will only be bound with DerefByRefResultName if we're dealing
+      // with a user-defined iterator type. Test the const qualification of
+      // the reference type.
+      Descriptor.DerefByConstRef = (*DerefType)
+                                       ->getAs<ReferenceType>()
+                                       ->getPointeeType()
+                                       .isConstQualified();
+    } else {
+      // By nature of the matcher this case is triggered only for built-in
+      // iterator types (i.e. pointers).
+      assert(isa<PointerType>(CanonicalInitVarType) &&
+             "Non-class iterator type is not a pointer type");
 
-  doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr),
-               ContainerString, Finder.getUsages(), Finder.getAliasDecl(),
-               Finder.aliasUseRequired(), Finder.aliasFromForInit(), TheLoop,
-               Descriptor);
+      // We test for const qualification of the pointed-at type.
+      Descriptor.DerefByConstRef =
+          CanonicalInitVarType->getPointeeType().isConstQualified();
     }
-
-void LoopConvertCheck::registerMatchers(MatchFinder *Finder) {
-  // Only register the matchers for C++. Because this checker is used for
-  // modernization, it is reasonable to run it on any C++ standard with the
-  // assumption the user is trying to modernize their codebase.
-  if (getLangOpts().CPlusPlus) {
-    Finder->addMatcher(makeArrayLoopMatcher(), this);
-    Finder->addMatcher(makeIteratorLoopMatcher(), this);
-    Finder->addMatcher(makePseudoArrayLoopMatcher(), this);
   }
 }
 
-void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
-  const BoundNodes &Nodes = Result.Nodes;
-  Confidence ConfidenceLevel(Confidence::CL_Safe);
-  ASTContext *Context = Result.Context;
-
-  const ForStmt *TheLoop;
-  LoopFixerKind FixerKind;
+/// \brief Determines the parameters needed to build the range replacement.
+void LoopConvertCheck::determineRangeDescriptor(
+    ASTContext *Context, const BoundNodes &Nodes, const ForStmt *Loop,
+    LoopFixerKind FixerKind, const Expr *ContainerExpr,
+    const UsageResult &Usages, RangeDescriptor &Descriptor) {
+  Descriptor.ContainerString = getContainerString(Context, Loop, ContainerExpr);
 
-  if ((TheLoop = Nodes.getStmtAs<ForStmt>(LoopNameArray))) {
-    FixerKind = LFK_Array;
-  } else if ((TheLoop = Nodes.getStmtAs<ForStmt>(LoopNameIterator))) {
-    FixerKind = LFK_Iterator;
-  } else {
-    TheLoop = Nodes.getStmtAs<ForStmt>(LoopNamePseudoArray);
-    assert(TheLoop && "Bad Callback. No for statement");
-    FixerKind = LFK_PseudoArray;
-  }
+  if (FixerKind == LFK_Iterator)
+    getIteratorLoopQualifiers(Context, Nodes, Descriptor);
+  else
+    getArrayLoopQualifiers(Context, Nodes, ContainerExpr, Usages, Descriptor);
 
+/// \brief Check some of the conditions that must be met for the loop to be
+/// convertible.
+bool LoopConvertCheck::isConvertible(ASTContext *Context,
+                                     const ast_matchers::BoundNodes &Nodes,
+                                     const ForStmt *Loop,
+                                     LoopFixerKind FixerKind) {
+  // If we already modified the range of this for loop, don't do any further
+  // updates on this iteration.
+  if (TUInfo->getReplacedVars().count(Loop))
+    return false;
+
   // Check that we have exactly one index variable and at most one end variable.
   const auto *LoopVar = Nodes.getDeclAs<VarDecl>(IncrementVarName);
   const auto *CondVar = Nodes.getDeclAs<VarDecl>(ConditionVarName);
   const auto *InitVar = Nodes.getDeclAs<VarDecl>(InitVarName);
   if (!areSameVariable(LoopVar, CondVar) || !areSameVariable(LoopVar, InitVar))
-    return;
+    return false;
   const auto *EndVar = Nodes.getDeclAs<VarDecl>(EndVarName);
   const auto *ConditionEndVar = Nodes.getDeclAs<VarDecl>(ConditionEndVarName);
   if (EndVar && !areSameVariable(EndVar, ConditionEndVar))
-    return;
-
-  // If the end comparison isn't a variable, we can try to work with the
-  // expression the loop variable is being tested against instead.
-  const auto *EndCall = Nodes.getStmtAs<CXXMemberCallExpr>(EndCallName);
-  const auto *BoundExpr = Nodes.getStmtAs<Expr>(ConditionBoundName);
-
-  // If the loop calls end()/size() after each iteration, lower our confidence
-  // level.
-  if (FixerKind != LFK_Array && !EndVar)
-    ConfidenceLevel.lowerTo(Confidence::CL_Reasonable);
+    return false;
 
-  const Expr *ContainerExpr = nullptr;
-  RangeDescriptor Descriptor{false, false, false, false};
-  // FIXME: Try to put most of this logic inside a matcher. Currently, matchers
-  // don't allow the ight-recursive checks in digThroughConstructors.
+  // FIXME: Try to put most of this logic inside a matcher.
   if (FixerKind == LFK_Iterator) {
-    ContainerExpr = findContainer(Context, LoopVar->getInit(),
-                                  EndVar ? EndVar->getInit() : EndCall,
-                                  &Descriptor.ContainerNeedsDereference);
-
     QualType InitVarType = InitVar->getType();
     QualType CanonicalInitVarType = InitVarType.getCanonicalType();
 
     const auto *BeginCall = Nodes.getNodeAs<CXXMemberCallExpr>(BeginCallName);
     assert(BeginCall && "Bad Callback. No begin call expression");
     QualType CanonicalBeginType =
         BeginCall->getMethodDecl()->getReturnType().getCanonicalType();
     if (CanonicalBeginType->isPointerType() &&
         CanonicalInitVarType->isPointerType()) {
-      QualType BeginPointeeType = CanonicalBeginType->getPointeeType();
-      QualType InitPointeeType = CanonicalInitVarType->getPointeeType();
       // If the initializer and the variable are both pointers check if the
-      // un-qualified pointee types match otherwise we don't use auto.
-      if (!Context->hasSameUnqualifiedType(InitPointeeType, BeginPointeeType))
-        return;
-      Descriptor.IsTriviallyCopyable =
-          BeginPointeeType.isTriviallyCopyableType(*Context);
-    } else {
+      // un-qualified pointee types match, otherwise we don't use auto.
+      if (!Context->hasSameUnqualifiedType(
+              CanonicalBeginType->getPointeeType(),
+              CanonicalInitVarType->getPointeeType()))
+        return false;
+    } else if (!Context->hasSameType(CanonicalInitVarType,
+                                     CanonicalBeginType)) {
       // Check for qualified types to avoid conversions from non-const to const
       // iterator types.
-      if (!Context->hasSameType(CanonicalInitVarType, CanonicalBeginType))
-        return;
+      return false;
     }
-
-    const auto *DerefByValueType =
-        Nodes.getNodeAs<QualType>(DerefByValueResultName);
-    Descriptor.DerefByValue = DerefByValueType;
-    if (!Descriptor.DerefByValue) {
-      if (const auto *DerefType =
-              Nodes.getNodeAs<QualType>(DerefByRefResultName)) {
-        // A node will only be bound with DerefByRefResultName if we're dealing
-        // with a user-defined iterator type. Test the const qualification of
-        // the reference type.
-        Descriptor.DerefByConstRef = (*DerefType)
-                                         ->getAs<ReferenceType>()
-                                         ->getPointeeType()
-                                         .isConstQualified();
-      } else {
-        // By nature of the matcher this case is triggered only for built-in
-        // iterator types (i.e. pointers).
-        assert(isa<PointerType>(CanonicalInitVarType) &&
-               "Non-class iterator type is not a pointer type");
-        QualType InitPointeeType = CanonicalInitVarType->getPointeeType();
-        QualType BeginPointeeType = CanonicalBeginType->getPointeeType();
-        // If the initializer and variable have both the same type just use auto
-        // otherwise we test for const qualification of the pointed-at type.
-        if (!Context->hasSameType(InitPointeeType, BeginPointeeType))
-          Descriptor.DerefByConstRef = InitPointeeType.isConstQualified();
-      }
-    } else {
-      // If the dereference operator returns by value then test for the
-      // canonical const qualification of the init variable type.
-      Descriptor.DerefByConstRef = CanonicalInitVarType.isConstQualified();
-      Descriptor.IsTriviallyCopyable =
-          DerefByValueType->isTriviallyCopyableType(*Context);
-    }
-  } else if (FixerKind == LFK_PseudoArray) {
-    if (!EndCall)
-      return;
+  } else if (FixerKind == LFK_PseudoArray) {
+    // This call is required to obtain the container.
+    const auto *EndCall = Nodes.getStmtAs<CXXMemberCallExpr>(EndCallName);
+    if (!EndCall || !dyn_cast<MemberExpr>(EndCall->getCallee()))
+      return false;
+  return true;
+}
+
+void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
+  const BoundNodes &Nodes = Result.Nodes;
+  Confidence ConfidenceLevel(Confidence::CL_Safe);
+  ASTContext *Context = Result.Context;
+
+  const ForStmt *Loop;
+  LoopFixerKind FixerKind;
+  RangeDescriptor Descriptor;
+
+  if ((Loop = Nodes.getStmtAs<ForStmt>(LoopNameArray))) {
+    FixerKind = LFK_Array;
+  } else if ((Loop = Nodes.getStmtAs<ForStmt>(LoopNameIterator))) {
+    FixerKind = LFK_Iterator;
+    Loop = Nodes.getStmtAs<ForStmt>(LoopNamePseudoArray);
+    assert(Loop && "Bad Callback. No for statement");
+    FixerKind = LFK_PseudoArray;
+
+  if (!isConvertible(Context, Nodes, Loop, FixerKind))
+
+  const auto *LoopVar = Nodes.getDeclAs<VarDecl>(IncrementVarName);
+  const auto *EndVar = Nodes.getDeclAs<VarDecl>(EndVarName);
+
+  // If the loop calls end()/size() after each iteration, lower our confidence
+  // level.
+  if (FixerKind != LFK_Array && !EndVar)
+    ConfidenceLevel.lowerTo(Confidence::CL_Reasonable);
+
+  // If the end comparison isn't a variable, we can try to work with the
+  // expression the loop variable is being tested against instead.
+  const auto *EndCall = Nodes.getStmtAs<CXXMemberCallExpr>(EndCallName);
+  const auto *BoundExpr = Nodes.getStmtAs<Expr>(ConditionBoundName);
+
+  // Find container expression of iterators and pseudoarrays, and determine if
+  // this expression needs to be dereferenced to obtain the container.
+  // With array loops, the container is often discovered during the
+  // ForLoopIndexUseVisitor traversal.
+  const Expr *ContainerExpr = nullptr;
+  if (FixerKind == LFK_Iterator) {
+    ContainerExpr = findContainer(Context, LoopVar->getInit(),
+                                  EndVar ? EndVar->getInit() : EndCall,
+                                  &Descriptor.ContainerNeedsDereference);
+  } else if (FixerKind == LFK_PseudoArray) {
     ContainerExpr = EndCall->getImplicitObjectArgument();
-    const auto *Member = dyn_cast<MemberExpr>(EndCall->getCallee());
-    if (!Member)
-      return;
-    Descriptor.ContainerNeedsDereference = Member->isArrow();
+    Descriptor.ContainerNeedsDereference =
+        dyn_cast<MemberExpr>(EndCall->getCallee())->isArrow();
   }
 
   // We must know the container or an array length bound.
   if (!ContainerExpr && !BoundExpr)
     return;
 
-  if (ConfidenceLevel.getLevel() < MinConfidence)
+  ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
+                                BoundExpr,
+                                Descriptor.ContainerNeedsDereference);
+
+  // Find expressions and variables on which the container depends.
+  if (ContainerExpr) {
+    ComponentFinderASTVisitor ComponentFinder;
+    ComponentFinder.findExprComponents(ContainerExpr->IgnoreParenImpCasts());
+    Finder.addComponents(ComponentFinder.getComponents());
+  }
+
+  // Find usages of the loop index. If they are not used in a convertible way,
+  // stop here.
+  if (!Finder.findAndVerifyUsages(Loop->getBody()))
+    return;
+  ConfidenceLevel.lowerTo(Finder.getConfidenceLevel());
+
+  // Obtain the container expression, if we don't have it yet.
+  if (FixerKind == LFK_Array) {
+    ContainerExpr = Finder.getContainerIndexed()->IgnoreParenImpCasts();
+
+    // Very few loops are over expressions that generate arrays rather than
+    // array variables. Consider loops over arrays that aren't just represented
+    // by a variable to be risky conversions.
+    if (!getReferencedVariable(ContainerExpr) &&
+        !isDirectMemberExpr(ContainerExpr))
+      ConfidenceLevel.lowerTo(Confidence::CL_Risky);
+  }
+
+  // Find out which qualifiers we have to use in the loop range.
+  const UsageResult &Usages = Finder.getUsages();
+  determineRangeDescriptor(Context, Nodes, Loop, FixerKind, ContainerExpr,
+                           Usages, Descriptor);
+
+  // Ensure that we do not try to move an expression dependent on a local
+  // variable declared inside the loop outside of it.
+  // FIXME: Determine when the external dependency isn't an expression converted
+  // by another loop.
+  TUInfo->getParentFinder().gatherAncestors(Context->getTranslationUnitDecl());
+  DependencyFinderASTVisitor DependencyFinder(
+      &TUInfo->getParentFinder().getStmtToParentStmtMap(),
+      &TUInfo->getParentFinder().getDeclToParentStmtMap(),
+      &TUInfo->getReplacedVars(), Loop);
+
+  if (DependencyFinder.dependsOnInsideVariable(ContainerExpr) ||
+      Descriptor.ContainerString.empty() || Usages.empty() ||
+      ConfidenceLevel.getLevel() < MinConfidence)
     return;
 
-  findAndVerifyUsages(Context, LoopVar, EndVar, ContainerExpr, BoundExpr,
-                      TheLoop, FixerKind, Descriptor);
+  doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr), Usages,
+               Finder.getAliasDecl(), Finder.aliasUseRequired(),
+               Finder.aliasFromForInit(), Loop, Descriptor);
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

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

for (iterator I = begin(); I != end(); ++I) {
       doSomething();
     }
   }
 
   void doLoop() const {
     for (const_iterator I = begin(), E = end(); I != E; ++I)
       (void) *I;
     // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead
-    // CHECK-FIXES: for (auto & elem : *this)
+    // CHECK-FIXES: for (const auto & elem : *this)
 
     for (const_iterator I = C::begin(), E = C::end(); I != E; ++I)
       (void) *I;
     // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead
-    // CHECK-FIXES: for (auto & elem : *this)
+    // CHECK-FIXES: for (const auto & elem : *this)
 
     for (const_iterator I = begin(), E = end(); I != E; ++I) {
       (void) *I;
       doSomething();
     }
   }
 };