[clang-tidy] Refactor loop-convert to bring most of the checking into matchers

[njames93]

Summary

Move all checking on the for loop declaration into the matchers.
Once a match is generated clients only need to examine the body of the for loop to ensure its convertible.
This also brings the handling of match results for Iterator and PseudoArray loops much more inline.

This supercedes D97889

[steveire]

Good progress! This check also implements its own StmtAncestorASTVisitor which should probably be removed in favor of the ParentMapContext eventually (not in this patch).

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

118

There's already a (deprecated) hasDefaultArgument in ASTMatchers.h with the same implementation.

It is deprecated in favor of hasInitializer.

152

Why not use lambdas?

236

It always makes sense to use auto for matchers. Here, you hide an unnecessary implicit conversion from BindableMatcher<Stmt> to Matcher<Stmt>.

Below you use auto for a matcher. Why be inconsistent?

859

Why change auto to VarDecl (repeated) here?

[njames93]

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

859

Changed it when I removed the initializer, forget to undo it when I added the initializer back.

[njames93]

Update.

[njames93]

Ping?

[njames93]

Remove unnecessary test case.

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

private:
 
   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);
-
   StringRef getReverseFunction() const;
   StringRef getReverseHeader() const;
 
   std::unique_ptr<TUTrackingInfo> TUInfo;
   const unsigned long long MaxCopySize;
   const Confidence::Level MinConfidence;
   const VariableNamer::NamingStyle NamingStyle;
   utils::IncludeInserter Inserter;

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

 
 namespace modernize {
 
 static const char LoopNameArray[] = "forLoopArray";
 static const char LoopNameIterator[] = "forLoopIterator";
 static const char LoopNameReverseIterator[] = "forLoopReverseIterator";
 static const char LoopNamePseudoArray[] = "forLoopPseudoArray";
 static const char ConditionBoundName[] = "conditionBound";
+static const char ContainerMemberExpr[] = "containerMemberExpr";
 static const char InitVarName[] = "initVar";
-static const char BeginCallName[] = "beginCall";
-static const char EndCallName[] = "endCall";
 static const char EndVarName[] = "endVar";
 static const char DerefByValueResultName[] = "derefByValueResult";
 static const char DerefByRefResultName[] = "derefByRefResult";
 
 static const StatementMatcher integerComparisonMatcher() {
   return expr(ignoringParenImpCasts(
       declRefExpr(to(varDecl(equalsBoundNode(InitVarName))))));
 }
return forStmt(
                  binaryOperator(hasOperatorName(">"), hasLHS(ArrayBoundMatcher),
                                 hasRHS(integerComparisonMatcher())))),
              hasIncrement(
                  unaryOperator(hasOperatorName("++"),
                                hasUnaryOperand(incrementVarMatcher()))))
       .bind(LoopNameArray);
 }
 
+namespace {
+
+AST_MATCHER(ParmVarDecl, hasDefaultArg) {
+  return Node.getDefaultArg() != nullptr;
+}
+
+AST_MATCHER(VarDecl, isCStyleInit) {
+  return Node.getInitStyle() == VarDecl::CInit;
+}
+
+AST_MATCHER(VarDecl, samePtrTypeAsBeginResult) {
+  struct Predicate {
+    bool operator()(const ast_matchers::internal::BoundNodesMap &Nodes) const {
+      const auto *BeginMember =
+          Nodes.getNodeAs<MemberExpr>(ContainerMemberExpr);
+      assert(BeginMember);
+      QualType RetType = cast<CXXMethodDecl>(BeginMember->getMemberDecl())
+                             ->getReturnType()
+                             .getCanonicalType();
+      if (RetType->isPointerType())
+        return !Context.hasSameUnqualifiedType(VarType->getPointeeType(),
+                                               RetType->getPointeeType());
+      return false;
+    }
+
+    QualType VarType;
+    const ASTContext &Context;
+  };
+  QualType VarType = Node.getType().getCanonicalType();
+  if (!VarType->isPointerType())
+    return true;
+  return Builder->removeBindings(Predicate{VarType, Finder->getASTContext()});
+}
+
+AST_MATCHER(MemberExpr, matchesBoundBeginContainer) {
+  struct Predicate {
+    bool operator()(const ast_matchers::internal::BoundNodesMap &Nodes) const {
+      const auto *BeginMember =
+          Nodes.getNodeAs<MemberExpr>(ContainerMemberExpr);
+      assert(BeginMember);
+      if (BeginMember->isArrow() != Node.isArrow())
+        return true;
+
+      return !areSameExpr(Context, BeginMember->getBase(), Node.getBase());
+    }
+
+    const MemberExpr &Node;
+    ASTContext *Context;
+  };
+  return Builder->removeBindings(Predicate{Node, &Finder->getASTContext()});
+}
+} // namespace
+
 /// The matcher used for iterator-based for loops.
 ///
 /// This matcher is more flexible than array-based loops. It will match
 /// catch loops of the following textual forms (regardless of whether the
 /// iterator type is actually a pointer type or a class type):
 ///
 /// \code
 ///   for (containerType::iterator it = container.begin(),
-///        e = createIterator(); it != e; ++it) { ... }
+///        e = container.end(); it != e; ++it) { ... }
 ///   for (containerType::iterator it = container.begin();
-///        it != anotherContainer.end(); ++it) { ... }
+///        it != container.end(); ++it) { ... }
 /// \endcode
 /// The following string identifiers are bound to the parts of the AST:
 ///   InitVarName: 'it' (as a VarDecl)
 ///   LoopName: The entire for loop (as a ForStmt)
+///   ContainerMemberExpr: 'container.begin' (as a MemberExpr)
 ///   In the first example only:
 ///     EndVarName: 'e' (as a VarDecl)
-///   In the second example only:
-///     EndCallName: 'container.end()' (as a CXXMemberCallExpr)
 ///
 /// Client code will need to make sure that:
-///   - The two containers on which 'begin' and 'end' are called are the same.
+///   - 'it' is only accessed using the '*' or '->' operators.
+///   - The container's iterators would not be invalidated during the loop.
+///
+/// This will not match cases such as:
+/// \code
+///   for (containerType::iterator it = container.begin(),
+///        e = differentContainer.end(); it != e; ++it) { ... }
+///   for (containerType::iterator it = container.begin(),
+///        e = container.end(); it != f; ++it) { ... }
+///   for (containerType::iterator it = container.myBegin(),
+///        e = container.myEnd(); it != e; ++it) { ... }
+///   for (containerType::iterator it = container.begin(),
+///        e = container->end(); it != e; ++it) { ... }
+///   for (containerType::iterator it = container.begin(),
+///        e = container.end(); it != e; ++e) { ... }
+/// \endcode
 StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
 
   auto BeginNameMatcher = IsReverse ? hasAnyName("rbegin", "crbegin")
                                     : hasAnyName("begin", "cbegin");
 
   auto EndNameMatcher =
       IsReverse ? hasAnyName("rend", "crend") : hasAnyName("end", "cend");
 
-  StatementMatcher BeginCallMatcher =
-      cxxMemberCallExpr(argumentCountIs(0),
-                        callee(cxxMethodDecl(BeginNameMatcher)))
-          .bind(BeginCallName);
+  StatementMatcher BeginCallMatcher = cxxMemberCallExpr(
+      argumentCountIs(0),
+      callee(memberExpr(member(cxxMethodDecl(BeginNameMatcher)))
+                 .bind(ContainerMemberExpr)));
+
+  auto DeclMatcher = [](auto MemberCallMatcher) {
+    // IgnoreUnlessSpelledInSource cant handle `Iterator
+    // It(getImplicitlyConvertableToIterator());` So Checking InitStyle does the
+    // job.
+    return varDecl(traverse(
+        TK_IgnoreUnlessSpelledInSource,
+        anyOf(
+            allOf(unless(isCStyleInit()),
+                  hasInitializer(anyOf(cxxConstructExpr(argumentCountIs(1),
+                                                        has(MemberCallMatcher)),
+                                       MemberCallMatcher))),
+            allOf(isCStyleInit(), hasInitializer(MemberCallMatcher)))));
+  };
 
   DeclarationMatcher InitDeclMatcher =
-      varDecl(hasInitializer(anyOf(ignoringParenImpCasts(BeginCallMatcher),
-                                   materializeTemporaryExpr(
-                                       ignoringParenImpCasts(BeginCallMatcher)),
-                                   hasDescendant(BeginCallMatcher))))
+      varDecl(DeclMatcher(BeginCallMatcher), samePtrTypeAsBeginResult())
           .bind(InitVarName);
 
-  DeclarationMatcher EndDeclMatcher =
-      varDecl(hasInitializer(anything())).bind(EndVarName);
-
-  StatementMatcher EndCallMatcher = cxxMemberCallExpr(
-      argumentCountIs(0), callee(cxxMethodDecl(EndNameMatcher)));
+  StatementMatcher EndCallMatcher =
+      cxxMemberCallExpr(argumentCountIs(0),
+                        callee(memberExpr(member(cxxMethodDecl(EndNameMatcher)),
+                                          matchesBoundBeginContainer())));
 
+  DeclarationMatcher EndDeclMatcher =
+      DeclMatcher(EndCallMatcher).bind(EndVarName);
   StatementMatcher IteratorBoundMatcher =
-      expr(anyOf(ignoringParenImpCasts(
-                     declRefExpr(to(varDecl(equalsBoundNode(EndVarName))))),
-                 ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)),
-                 materializeTemporaryExpr(ignoringParenImpCasts(
-                     expr(EndCallMatcher).bind(EndCallName)))));
+      expr(traverse(TK_IgnoreUnlessSpelledInSource,
+                    anyOf(declRefExpr(to(varDecl(equalsBoundNode(EndVarName)))),
+                          expr(EndCallMatcher))));
 
   StatementMatcher IteratorComparisonMatcher = expr(ignoringParenImpCasts(
       declRefExpr(to(varDecl(equalsBoundNode(InitVarName))))));
 
   // This matcher tests that a declaration is a CXXRecordDecl that has an
   // overloaded operator*(). If the operator*() returns by value instead of by
   // reference then the return type is tagged with DerefByValueResultName.
-  internal::Matcher<VarDecl> TestDerefReturnsByValue =
+  ast_matchers::internal::Matcher<VarDecl> TestDerefReturnsByValue =
       hasType(hasUnqualifiedDesugaredType(
           recordType(hasDeclaration(cxxRecordDecl(hasMethod(cxxMethodDecl(
               hasOverloadedOperatorName("*"),
               anyOf(
                   // Tag the return type if it's by value.
                   returns(qualType(unless(hasCanonicalType(referenceType())))
                               .bind(DerefByValueResultName)),
                   returns(
return forStmt(
                                                 TestDerefReturnsByValue))))))))
       .bind(IsReverse ? LoopNameReverseIterator : LoopNameIterator);
 }
 
 /// The matcher used for array-like containers (pseudoarrays).
 ///
 /// This matcher is more flexible than array-based loops. It will match
 /// loops of the following textual forms (regardless of whether the
-/// iterator type is actually a pointer type or a class type):
+/// iterator type is actually a pointer type or a class type).
+/// The container matched must have begin and end methods that either have no
+/// params or only default params. If the container is a const object, these
+/// methods must also be marked const:
 ///
 /// \code
 ///   for (int i = 0, j = container.size(); i < j; ++i) { ... }
 ///   for (int i = 0; i < container.size(); ++i) { ... }
+///   for (int i = 0; i < container.length(); ++i) { ... }
 /// \endcode
 /// The following string identifiers are bound to the parts of the AST:
 ///   InitVarName: 'i' (as a VarDecl)
 ///   LoopName: The entire for loop (as a ForStmt)
-///   In the first example only:
+///   ContainerMemberExpr: 'container.size' or 'container.length' (as a
+///   MemberExpr) In the first example only:
 ///     EndVarName: 'j' (as a VarDecl)
-///   In the second example only:
-///     EndCallName: 'container.size()' (as a CXXMemberCallExpr)
 ///
 /// Client code will need to make sure that:
 ///   - The containers on which 'size()' is called is the container indexed.
 ///   - The index variable is only used in overloaded operator[] or
 ///     container.at().
 ///   - The container's iterators would not be invalidated during the loop.
+///
+/// This will not match cases such as:
+/// \code
+///   for (int i = 0, j = container.size(); i < k; ++i) { ... }
+///   for (int i = 0, j = container.size(); i < j; ++j) { ... }
+///   for (int i = 0, j = container.size(); k < j; ++i) { ... }
+///   for (int i = 0, j = container.size(); i <= j; ++i) { ... }
+/// \endcode
 StatementMatcher makePseudoArrayLoopMatcher() {
   // Test that the incoming type has a record declaration that has methods
   // called 'begin' and 'end'. If the incoming type is const, then make sure
   // these methods are also marked const.
   //
   // FIXME: To be completely thorough this matcher should also ensure the
   // return type of begin/end is an iterator that dereferences to the same as
   // what operator[] or at() returns. Such a test isn't likely to fail except
   // for pathological cases.
   //
   // FIXME: Also, a record doesn't necessarily need begin() and end(). Free
   // functions called begin() and end() taking the container as an argument
   // are also allowed.
+
+  auto NoMandatoryParams =
+      anyOf(parameterCountIs(0), hasParameter(0, hasDefaultArg()));
+
   TypeMatcher RecordWithBeginEnd = qualType(anyOf(
       qualType(
           isConstQualified(),
           hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
-              hasMethod(cxxMethodDecl(hasName("begin"), isConst())),
-              hasMethod(cxxMethodDecl(hasName("end"),
+              hasMethod(cxxMethodDecl(NoMandatoryParams, hasName("begin"),
+                                      isConst())),
+              hasMethod(cxxMethodDecl(NoMandatoryParams, hasName("end"),
                                       isConst()))))   // hasDeclaration
                                                  ))), // qualType
-      qualType(unless(isConstQualified()),
-               hasUnqualifiedDesugaredType(recordType(hasDeclaration(
-                   cxxRecordDecl(hasMethod(hasName("begin")),
-                                 hasMethod(hasName("end"))))))) // qualType
+      qualType(
+          unless(isConstQualified()),
+          hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
+              hasMethod(allOf(NoMandatoryParams, hasName("begin"))),
+              hasMethod(
+                  allOf(NoMandatoryParams, hasName("end")))))))) // qualType
       ));
 
   StatementMatcher SizeCallMatcher = cxxMemberCallExpr(
-      argumentCountIs(0), callee(cxxMethodDecl(hasAnyName("size", "length"))),
+      argumentCountIs(0),
+      callee(memberExpr(member(cxxMethodDecl(hasAnyName("size", "length"))))
+                 .bind(ContainerMemberExpr)),
       on(anyOf(hasType(pointsTo(RecordWithBeginEnd)),
                hasType(RecordWithBeginEnd))));
 
   StatementMatcher EndInitMatcher =
-      expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)),
-                 explicitCastExpr(hasSourceExpression(ignoringParenImpCasts(
-                     expr(SizeCallMatcher).bind(EndCallName))))));
+      expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher)),
+                 explicitCastExpr(hasSourceExpression(
+                     ignoringParenImpCasts(expr(SizeCallMatcher))))));
 
   DeclarationMatcher EndDeclMatcher =
       varDecl(hasInitializer(EndInitMatcher)).bind(EndVarName);
 
   StatementMatcher IndexBoundMatcher =
       expr(anyOf(ignoringParenImpCasts(
                      declRefExpr(to(varDecl(equalsBoundNode(EndVarName))))),
                  EndInitMatcher));
return forStmt(
                  binaryOperator(hasOperatorName(">"), hasLHS(IndexBoundMatcher),
                                 hasRHS(integerComparisonMatcher())))),
              hasIncrement(
                  unaryOperator(hasOperatorName("++"),
                                hasUnaryOperand(incrementVarMatcher()))))
       .bind(LoopNamePseudoArray);
 }
 
-/// Determine whether Init appears to be an initializing an iterator.
-///
-/// If it is, returns the object whose begin() or end() method is called, and
-/// the output parameter isArrow is set to indicate whether the initialization
-/// is called via . or ->.
-static const Expr *getContainerFromBeginEndCall(const Expr *Init, bool IsBegin,
-                                                bool *IsArrow, bool IsReverse) {
-  // FIXME: Maybe allow declaration/initialization outside of the for loop.
-  const auto *TheCall =
-      dyn_cast_or_null<CXXMemberCallExpr>(digThroughConstructors(Init));
-  if (!TheCall || TheCall->getNumArgs() != 0)
-    return nullptr;
-
-  const auto *Member = dyn_cast<MemberExpr>(TheCall->getCallee());
-  if (!Member)
-    return nullptr;
-  StringRef Name = Member->getMemberDecl()->getName();
-  if (!Name.consume_back(IsBegin ? "begin" : "end"))
-    return nullptr;
-  if (IsReverse && !Name.consume_back("r"))
-    return nullptr;
-  if (!Name.empty() && !Name.equals("c"))
-    return nullptr;
-
-  const Expr *SourceExpr = Member->getBase();
-  if (!SourceExpr)
-    return nullptr;
-
-  *IsArrow = Member->isArrow();
-  return SourceExpr;
-}
-
-/// Determines the container whose begin() and end() functions are called
-/// for an iterator-based loop.
-///
-/// BeginExpr must be a member call to a function named "begin()", and EndExpr
-/// must be a member.
-static const Expr *findContainer(ASTContext *Context, const Expr *BeginExpr,
-                                 const Expr *EndExpr,
-                                 bool *ContainerNeedsDereference,
-                                 bool IsReverse) {
-  // Now that we know the loop variable and test expression, make sure they are
-  // valid.
-  bool BeginIsArrow = false;
-  bool EndIsArrow = false;
-  const Expr *BeginContainerExpr = getContainerFromBeginEndCall(
-      BeginExpr, /*IsBegin=*/true, &BeginIsArrow, IsReverse);
-  if (!BeginContainerExpr)
-    return nullptr;
-
-  const Expr *EndContainerExpr = getContainerFromBeginEndCall(
-      EndExpr, /*IsBegin=*/false, &EndIsArrow, IsReverse);
-  // Disallow loops that try evil things like this (note the dot and arrow):
-  //  for (IteratorType It = Obj.begin(), E = Obj->end(); It != E; ++It) { }
-  if (!EndContainerExpr || BeginIsArrow != EndIsArrow ||
-      !areSameExpr(Context, EndContainerExpr, BeginContainerExpr))
-    return nullptr;
-
-  *ContainerNeedsDereference = BeginIsArrow;
-  return BeginContainerExpr;
-}
-
 /// Obtain the original source code text from a SourceRange.
 static StringRef getStringFromRange(SourceManager &SourceMgr,
                                     const LangOptions &LangOpts,
                                     SourceRange Range) {
   if (SourceMgr.getFileID(Range.getBegin()) !=
       SourceMgr.getFileID(Range.getEnd())) {
     return StringRef(); // Empty string.
   }
void LoopConvertCheck::determineRangeDescriptor(
   Descriptor.NeedsReverseCall = (FixerKind == LFK_ReverseIterator);
 
   if (FixerKind == LFK_Iterator || FixerKind == LFK_ReverseIterator)
     getIteratorLoopQualifiers(Context, Nodes, Descriptor);
   else
     getArrayLoopQualifiers(Context, Nodes, ContainerExpr, Usages, Descriptor);
 }
 
-/// 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 *InitVar = Nodes.getNodeAs<VarDecl>(InitVarName);
-
-  // FIXME: Try to put most of this logic inside a matcher.
-  if (FixerKind == LFK_Iterator || FixerKind == LFK_ReverseIterator) {
-    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()) {
-      // 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(
-              CanonicalBeginType->getPointeeType(),
-              CanonicalInitVarType->getPointeeType()))
-        return false;
-    }
-  } else if (FixerKind == LFK_PseudoArray) {
-    // This call is required to obtain the container.
-    const auto *EndCall = Nodes.getNodeAs<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.getNodeAs<ForStmt>(LoopNameArray))) {
     FixerKind = LFK_Array;
   } else if ((Loop = Nodes.getNodeAs<ForStmt>(LoopNameIterator))) {
     FixerKind = LFK_Iterator;
   } else if ((Loop = Nodes.getNodeAs<ForStmt>(LoopNameReverseIterator))) {
     FixerKind = LFK_ReverseIterator;
   } else {
     Loop = Nodes.getNodeAs<ForStmt>(LoopNamePseudoArray);
     assert(Loop && "Bad Callback. No for statement");
     FixerKind = LFK_PseudoArray;
   }
 
-  if (!isConvertible(Context, Nodes, Loop, 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;
 
   const auto *LoopVar = Nodes.getNodeAs<VarDecl>(InitVarName);
-  const auto *EndVar = Nodes.getNodeAs<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);
+  const VarDecl *EndVar = Nodes.getNodeAs<VarDecl>(EndVarName);
 
   // 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.getNodeAs<CXXMemberCallExpr>(EndCallName);
-  const auto *BoundExpr = Nodes.getNodeAs<Expr>(ConditionBoundName);
+  const Expr *BoundExpr = nullptr;
 
   // 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 || FixerKind == LFK_ReverseIterator) {
-    ContainerExpr = findContainer(
-        Context, LoopVar->getInit(), EndVar ? EndVar->getInit() : EndCall,
-        &Descriptor.ContainerNeedsDereference,
-        /*IsReverse=*/FixerKind == LFK_ReverseIterator);
-  } else if (FixerKind == LFK_PseudoArray) {
-    ContainerExpr = EndCall->getImplicitObjectArgument();
-    Descriptor.ContainerNeedsDereference =
-        dyn_cast<MemberExpr>(EndCall->getCallee())->isArrow();
-  }
 
-  // We must know the container or an array length bound.
-  if (!ContainerExpr && !BoundExpr)
-    return;
+  if (FixerKind == LFK_Array) {
+    BoundExpr = Nodes.getNodeAs<Expr>(ConditionBoundName);
+    assert(BoundExpr &&
+           "Bad callback, ConditionBoundName must be bounded in Array loops");
+  } else {
+    // If the loop calls end()/size() after each iteration, lower our confidence
+    // level.
+    if (!EndVar)
+      ConfidenceLevel.lowerTo(Confidence::CL_Reasonable);
+    const auto *CE = Result.Nodes.getNodeAs<MemberExpr>(ContainerMemberExpr);
+    assert(
+        CE &&
+        "Bad callback, ContainerMemberExpr must be bounded in Container loops");
+    assert(CE->getBase());
+    ContainerExpr = CE->getBase();
+    Descriptor.ContainerNeedsDereference = CE->isArrow();
+  }
 
   ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
                                 BoundExpr,
                                 Descriptor.ContainerNeedsDereference);
 
   // Find expressions and variables on which the container depends.
   if (ContainerExpr) {
     ComponentFinderASTVisitor ComponentFinder;

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

void different_type() {
 
   for (dependent<int>::const_iterator It(V.begin()), E = V.end();
        It != E; ++It) {
     printf("Fibonacci number is %d\n", *It);
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (int It : V)
   // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
+
+  for (dependent<int>::const_iterator It{V.begin()}, E = V.end();
+       It != E; ++It) {
+    printf("Fibonacci number is %d\n", *It);
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHEfCK-FIXES: for (int It : V)
+  // CHECfK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
 // If member calls are made to 'this' within the loop, the transform becomes
 // risky as these calls may affect state that affects the loop.
 class C {
 public:
   typedef MutableVal *iterator;