[clang-tidy] Support begin/end free functions in modernize-loop-convert

[ccotter]

Summary

The modernize-loop-convert check will now match iterator based loops
that call the free functions 'begin'/'end', as well as matching the
free function 'size' on containers.

Test plan: Added unit test cases matching free function calls on
containers, and a single negative test case for length() which is not
supported.

[Eugene.Zelenko]

It'll be good idea to mention change in Release Notes.

[Eugene.Zelenko]

I just look into check documentation, new feature should be described with example there too.

[ccotter]

In D140760#4019680, @Eugene.Zelenko wrote:

I just look into check documentation, new feature should be described with example there too.

Would you mind clarifying where I should add documentation to? In the release notes, or within LoopConvertCheck.cpp itself?

[Eugene.Zelenko]

In D140760#4019708, @ccotter wrote:

In D140760#4019680, @Eugene.Zelenko wrote:

I just look into check documentation, new feature should be described with example there too.

Would you mind clarifying where I should add documentation to? In the release notes, or within LoopConvertCheck.cpp itself?

Sorry, if I was not clear. I meant clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst.

[ccotter]

Update docs

[ccotter]

Update tests

[ccotter]

oops, redo diff with correct arc diff command

[ccotter]

• Remove unused variable

[ccotter]

• [clang-tidy] Support begin/end free functions in modernize-loop-convert
• Add release note
• Update docs
• Fix tests
• Remove unused variable

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

219

Please keep alphabetical order (for check names) in this section.

[ccotter]

• alpha

[ccotter]

clang-tools-extra/docs/ReleaseNotes.rst

219

Done, and I fixed what looks like an existing out of order check in the release notes: https://reviews.llvm.org/D141391

[ccotter]

• fix merge

[ccotter]

Friendly ping.

[carlosgalvezp]

LGTM, minor comments. I'm not familiar with the implementation so I'm not very confident reviewing it, would be good to get some more expert eyes on it. Tests look solid!

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

317

Remove const, it's unused.

323

if (const auto *Member = ...)

[PiotrZSL]

In short, better support like this than lack of suport.
Consider adding option for disabling those free standing functions, reason why I'm telling this is that in case of false-positives, it may be easier just to disable part of functionality than disabling entire check or mass adding NOLINTs.

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

158–159

Now it will also catch calls to single parameter methods...
maybe unless(cxxMethodDecl()), after all we want to catch only functions

175

Now it will also catch calls to single parameter methods...
maybe unless(cxxMethodDecl()), after all we want to catch only functions

282

Consider exclude single parameter methods.

870

`else return Nodes.getNodeAs<CallExpr>(EndCallName) != nullptr;`

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

449

Also you could do a mix of member begin and free standing end ?
will it be supported or it wont ?

456

would be nice to validate if begin/end got same argument, but for that probably we would need to extract isIdenticalStmt function from clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp into separate matcher, so thats not a topic for this change.

[ccotter]

• Address feedback

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

317

good call..this is another check/cppcoreguideline anyway.

[ccotter]

• fix bad merge in ReleaseNotes

[ccotter]

• Add negative test case
• Fix logic to only consider std:: or ADL calls

[ccotter]

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

175

Updated this to properly only look at 1) a qualified call to std::begin etc, or 2) an ADL call to begin etc.

870

Is this in the style guide? NotNullTerminatedResultCheck.cpp, SignedCharMisuseCheck.cpp, UnconventionalAssignOperatorCheck.cpp have equivalent conditionals that do not check against nullptr.

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

449

No, I updated the logic and added test cases. I don't think mixing makes sense since range based loops look at symmetric member methods or ADL lookups but not mixed cases. This would not be a common case anyway in code I expect.

456

The existing logic handles this. I'll add a negative test case.

[ccotter]

I'm not sure about the false positive case. Range based for loops reduce to calls to ADL lookup begin/end (or member function calls to begin/end). I believe the same "false positive" argument (whatever that may be) would apply in an equivalent manner to the member call case.

[ccotter]

• Add comment regarding std::begin/end

[ccotter]

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

135

std::begin etc are singled out single they delegate to the member function begin, although they're really unrelated to ADL begin etc.

[PiotrZSL]

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

870

its not about checking for nullptr, its about simplifying if else if else, you dont need to explicitly compare to nullptr, I'm just used to it.
You can easily depend on implicit bool conversion here, its fine.
Simply you got if something return true else return false, you can write just return something.

[PiotrZSL]

consider adding testcase like this:

for (auto I = Obj.begin(5), E = obj.end(5); I != E; ++I) {
  ...
}

[ccotter]

• Add another test case

[ccotter]

• fix bad rebase

[ccotter]

• alpha

[PiotrZSL]

What about classes that doesn't have begin/end method but got cbegin/cend ?
I thing there is open issue for that.

[ccotter]

In D140760#4222989, @PiotrZSL wrote:

What about classes that doesn't have begin/end method but got cbegin/cend ?
I thing there is open issue for that.

Should we handle that in a separate patch?

[PiotrZSL]

In D140760#4226213, @ccotter wrote:

Should we handle that in a separate patch?

Ok.

[ccotter]

• format

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

158–159

I think usesADL() accomplishes the same thing here.

[ccotter]

bump - any other feedback?

[PiotrZSL]

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

73

consider: #include "llvm/ADT/StringSet.h" & llvm::StringSet

282

hasAnyName -> hasName, there is only 1 argument.

285

hasAnyName -> hasName, there is only 1 argument.

323

change this tuple into some struct, it's hard to find out what those arguments are...

331

STYLE: no need for if (xx) { y1 } else { y2 }, just if (xx) y1 else y2; should be sufficient

340

STYLE: this else is not needed, there are returns in previous anyway
simply if will be sufficient

345

count -> contains

345

no need for tuple for 2 argments, pair would be sufficient.

349

count -> contains

clang-tools-extra/docs/ReleaseNotes.rst

151–154

maybe just "Enhanced XYZ check to support for-loops with iterators initialized by free function calls like begin, end, or size."
I got lost on this part "to refactor container based for loops"

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

477

what if begin,end is missing, and there is only cbegin, cend then it will fail with:
"error: invalid range expression of type 'const A'; no viable 'begin' function available|

481

shouldnt it be const& It ?

720

add some tests for C++20 with rbegin, rend

[ccotter]

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

477

This looks to be an already existing bug in the check - mind if I fix that in a separate phab?

[ccotter]

Feedback and rebase

[ccotter]

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

481

Looks like when the object is trivial to copy (IsCheapToCopy logic in the check), auto It is used instead of const auto& It

[ccotter]

• Fix compile for non-libc++

[ccotter]

rebase

[PiotrZSL]

LGTM, things that are missing:

• some crash protection (getName)
• test for rbegin/rend

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

310

if we can, please use enum class

340

would be nice to verify if we got "name" here, same in other places

877

no need for else after return

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

477

Please add test with rbegin/rend

[ccotter]

• Feedback

[ccotter]

• Check nullptr
• Add tests for rbegin/crbegin

[ccotter]

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

477

Will do.

[PiotrZSL]

LGTM

[ccotter]

All done on my end - @PiotrZSL if you're good would you mind landing this for me?

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

 //===--- LoopConvertCheck.cpp - clang-tidy---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "LoopConvertCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <cstring>
+#include <tuple>
 #include <utility>
 
 using namespace clang::ast_matchers;
 using namespace llvm;
 
 namespace clang {
 namespace tidy {
 

 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))))));
 }
 
 static const DeclarationMatcher initToZeroMatcher() {
   return varDecl(
              hasInitializer(ignoringParenImpCasts(integerLiteral(equals(0)))))
       .bind(InitVarName);
 }
 

 /// 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) { ... }
 ///   for (containerType::iterator it = container.begin();
 ///        it != anotherContainer.end(); ++it) { ... }
+///   for (containerType::iterator it = begin(container),
+///        e = end(container); it != e; ++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)
 ///   In the first example only:
 ///     EndVarName: 'e' (as a VarDecl)
 ///   In the second example only:
-///     EndCallName: 'container.end()' (as a CXXMemberCallExpr)
+///     EndCallName: 'container.end()' (as a CXXMemberCallExpr) or
+///     'end(container)' (as a CallExpr)
 ///
 /// Client code will need to make sure that:
 ///   - The two containers on which 'begin' and 'end' are called are the same.
 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)))
+      callExpr(anyOf(cxxMemberCallExpr(argumentCountIs(0),
+                                       callee(cxxMethodDecl(BeginNameMatcher))),
+                     callExpr(argumentCountIs(1),
+                              callee(functionDecl(BeginNameMatcher)))))
           .bind(BeginCallName);
 
   DeclarationMatcher InitDeclMatcher =
       varDecl(hasInitializer(anyOf(ignoringParenImpCasts(BeginCallMatcher),
                                    materializeTemporaryExpr(
                                        ignoringParenImpCasts(BeginCallMatcher)),
                                    hasDescendant(BeginCallMatcher))))
           .bind(InitVarName);
 
   DeclarationMatcher EndDeclMatcher =
       varDecl(hasInitializer(anything())).bind(EndVarName);
 
-  StatementMatcher EndCallMatcher = cxxMemberCallExpr(
-      argumentCountIs(0), callee(cxxMethodDecl(EndNameMatcher)));
+  StatementMatcher EndCallMatcher = expr(anyOf(
+      cxxMemberCallExpr(argumentCountIs(0),
+                        callee(cxxMethodDecl(EndNameMatcher))),
+      callExpr(argumentCountIs(1), callee(functionDecl(EndNameMatcher)))));
 
   StatementMatcher IteratorBoundMatcher =
       expr(anyOf(ignoringParenImpCasts(
                      declRefExpr(to(varDecl(equalsBoundNode(EndVarName))))),
                  ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)),
                  materializeTemporaryExpr(ignoringParenImpCasts(
                      expr(EndCallMatcher).bind(EndCallName)))));
 

 ///
 /// 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):
 ///
 /// \code
 ///   for (int i = 0, j = container.size(); i < j; ++i) { ... }
 ///   for (int i = 0; i < container.size(); ++i) { ... }
+///   for (int i = 0; i < size(container); ++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:
 ///     EndVarName: 'j' (as a VarDecl)
 ///   In the second example only:
-///     EndCallName: 'container.size()' (as a CXXMemberCallExpr)
+///     EndCallName: 'container.size()' (as a CXXMemberCallExpr) or
+///     'size(contaner)' (as a CallExpr)
 ///
 /// 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.
 StatementMatcher makePseudoArrayLoopMatcher() {
   // Test that the incoming type has a record declaration that has methods
TypeMatcher RecordWithBeginEnd = qualType(anyOf(
                                       isConst()))))   // hasDeclaration
                                                  ))), // qualType
       qualType(unless(isConstQualified()),
                hasUnqualifiedDesugaredType(recordType(hasDeclaration(
                    cxxRecordDecl(hasMethod(hasName("begin")),
                                  hasMethod(hasName("end"))))))) // qualType
       ));
 
-  StatementMatcher SizeCallMatcher = cxxMemberCallExpr(
-      argumentCountIs(0), callee(cxxMethodDecl(hasAnyName("size", "length"))),
+  StatementMatcher SizeCallMatcher = expr(anyOf(
+      cxxMemberCallExpr(argumentCountIs(0),
+                        callee(cxxMethodDecl(hasAnyName("size", "length"))),
                         on(anyOf(hasType(pointsTo(RecordWithBeginEnd)),
-               hasType(RecordWithBeginEnd))));
+                                 hasType(RecordWithBeginEnd)))),
+      callExpr(argumentCountIs(1), callee(functionDecl(hasAnyName("size"))))));
 
   StatementMatcher EndInitMatcher =
       expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)),
                  explicitCastExpr(hasSourceExpression(ignoringParenImpCasts(
                      expr(SizeCallMatcher).bind(EndCallName))))));
 
   DeclarationMatcher EndDeclMatcher =
       varDecl(hasInitializer(EndInitMatcher)).bind(EndVarName);
 
   StatementMatcher IndexBoundMatcher =
       expr(anyOf(ignoringParenImpCasts(
                      declRefExpr(to(varDecl(equalsBoundNode(EndVarName))))),
                  EndInitMatcher));
 
   return forStmt(unless(isInTemplateInstantiation()),
                  hasLoopInit(
                      anyOf(declStmt(declCountIs(2),
                                     containsDeclaration(0, initToZeroMatcher()),
                                     containsDeclaration(1, EndDeclMatcher)),
                            declStmt(hasSingleDecl(initToZeroMatcher())))),
                  hasCondition(arrayConditionMatcher(IndexBoundMatcher)),
                  hasIncrement(
                      unaryOperator(hasOperatorName("++"),
                                    hasUnaryOperand(incrementVarMatcher()))))
       .bind(LoopNamePseudoArray);
 }
 
+// Find the Expr likely initializing an iterator.
+//
+// Call is either a CXXMemberCallExpr ('c.begin()') or CallExpr of a free
+// function with the first argument as a container ('begin(c)'), or nullptr.
+// Returns at a 3-tuple with the container expr, function name (begin/end/etc),
+// and whether the call is made through an arrow (->) for CXXMemberCallExprs.
+// The returned Expr* is nullptr if any of the assumptions are not met.
+static const std::tuple<const Expr *, StringRef, bool>
+getContainerExpr(const Expr *Call) {
+  const Expr *Dug = digThroughConstructorsConversions(Call);
+
+  if (const auto *TheCall = dyn_cast_or_null<CXXMemberCallExpr>(Dug)) {
+    const auto *Member = dyn_cast<MemberExpr>(TheCall->getCallee());
+    if (!Member) {
+      return std::make_tuple(TheCall->getArg(0),
+                             TheCall->getDirectCallee()->getName(), false);
+    }
+    return std::make_tuple(TheCall->getImplicitObjectArgument(),
+                           Member->getMemberDecl()->getName(),
+                           Member->isArrow());
+  } else if (const auto *TheCall = dyn_cast_or_null<CallExpr>(Dug)) {
+    if (TheCall->getNumArgs() != 1)
+      return std::make_tuple(nullptr, StringRef{}, false);
+    return std::make_tuple(TheCall->getArg(0),
+                           TheCall->getDirectCallee()->getName(), false);
+  }
+  return std::make_tuple(nullptr, StringRef{}, false);
+}
+
 /// 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>(
-      digThroughConstructorsConversions(Init));
-  if (!TheCall || TheCall->getNumArgs() != 0)
-    return nullptr;
 
-  const auto *Member = dyn_cast<MemberExpr>(TheCall->getCallee());
-  if (!Member)
+  StringRef Name;
+  const Expr *ContainerExpr;
+  std::tie(ContainerExpr, Name, *IsArrow) = getContainerExpr(Init);
+  if (!ContainerExpr) {
     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;
+  return ContainerExpr;
 }
 
 /// 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,
StringRef LoopConvertCheck::getContainerString(ASTContext *Context,
                                                const Expr *ContainerExpr) {
   StringRef ContainerString;
   ContainerExpr = ContainerExpr->IgnoreParenImpCasts();
   if (isa<CXXThisExpr>(ContainerExpr)) {
     ContainerString = "this";
   } else {
     // For CXXOperatorCallExpr such as vector_ptr->size() we want the class
     // object vector_ptr, but for vector[2] we need the whole expression.
-    if (const auto* E = dyn_cast<CXXOperatorCallExpr>(ContainerExpr))
+    if (const auto *E = dyn_cast<CXXOperatorCallExpr>(ContainerExpr))
       if (E->getOperator() != OO_Subscript)
         ContainerExpr = E->getArg(0);
     ContainerString =
         getStringFromRange(Context->getSourceManager(), Context->getLangOpts(),
                            ContainerExpr->getSourceRange());
   }
 
   return ContainerString;
bool LoopConvertCheck::isConvertible(ASTContext *Context,
   // 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);
+    const auto *BeginCall = Nodes.getNodeAs<CallExpr>(BeginCallName);
     assert(BeginCall && "Bad Callback. No begin call expression");
     QualType CanonicalBeginType =
-        BeginCall->getMethodDecl()->getReturnType().getCanonicalType();
+        BeginCall->getDirectCallee()->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) {
+    if (const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName)) {
       // This call is required to obtain the container.
-    const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName);
-    if (!EndCall || !isa<MemberExpr>(EndCall->getCallee()))
+      if (!isa<MemberExpr>(EndCall->getCallee()))
         return false;
+    } else if (Nodes.getNodeAs<CallExpr>(EndCallName)) {
+      return true;
+    } else {
+      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;
void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
 
   // 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.getNodeAs<CXXMemberCallExpr>(EndCallName);
+  const auto *EndCall = Nodes.getNodeAs<Expr>(EndCallName);
   const auto *BoundExpr = Nodes.getNodeAs<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 || 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();
+    std::tie(ContainerExpr, std::ignore, Descriptor.ContainerNeedsDereference) =
+        getContainerExpr(EndCall);
   }
 
   // We must know the container or an array length bound.
   if (!ContainerExpr && !BoundExpr)
     return;
 
   ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
                                 BoundExpr,

clang-tools-extra/docs/ReleaseNotes.rst

 - New alias :doc:`cert-msc54-cpp
   <clang-tidy/checks/cert/msc54-cpp>` to
   :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` was added.
 
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Fixed a false positive in :doc:`bugprone-assignment-in-if-condition
   <clang-tidy/checks/bugprone/assignment-in-if-condition>` check when there
   was an assignement in a lambda found in the condition of an ``if``.
 
 - Improved :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` check. Partial
   support for C++14 signal handler rules was added. Bug report generation was
   improved.
 
 - Fixed a false positive in :doc:`cppcoreguidelines-pro-type-member-init
   <clang-tidy/checks/cppcoreguidelines/pro-type-member-init>` when warnings
- Improved :doc:`misc-redundant-expression <clang-tidy/checks/misc/redundant-expression>`
 
   The check now skips concept definitions since redundant expressions still make sense
   inside them.
 
 - Support removing ``c_str`` calls from ``std::string_view`` constructor calls in
   :doc: `readability-redundant-string-cstr <clang-tidy/checks/readability/redundant-string-cstr>`
   check.
 
+- Improved :doc:`modernize-loop-convert <clang-tidy/checks/modernize/loop-convert>` to
+  refactor container based for loops that initialize iterators with free function calls
+  to ``begin``, ``end``, or ``size``.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst

.. code-block:: c++
   for (int i = 0; i < N; ++i)
     cout << arr[i];
 
   // reasonable conversion
   for (vector<int>::iterator it = v.begin(); it != v.end(); ++it)
     cout << *it;
 
   // reasonable conversion
+  for (vector<int>::iterator it = begin(v); it != end(v); ++it)
+    cout << *it;
+
+  // reasonable conversion
   for (int i = 0; i < v.size(); ++i)
     cout << v[i];
 
+  // reasonable conversion
+  for (int i = 0; i < size(v); ++i)
+    cout << v[i];
+
 After applying the check with minimum confidence level set to `reasonable` (default):
 
 .. code-block:: c++
 
   const int N = 5;
   int arr[] = {1,2,3,4,5};
   vector<int> v;
   v.push_back(1);

clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h

 #ifndef STRUCTURES_H
 #define STRUCTURES_H
 
 extern "C" {
 extern int printf(const char *restrict, ...);
 }
 
 struct Val {int X; void g(); };
struct S {
   const_iterator begin() const;
   const_iterator end() const;
   const_iterator cbegin() const;
   const_iterator cend() const;
   iterator begin();
   iterator end();
 };
 
+S::const_iterator begin(const S& s);
+S::const_iterator end(const S& s);
+S::const_iterator cbegin(const S& s);
+S::const_iterator cend(const S& s);
+S::iterator begin(S& s);
+S::iterator end(S& s);
+unsigned size(const S& s);
+
 struct T {
   typedef int value_type;
   struct iterator {
     value_type &operator*();
     const value_type &operator*() const;
     iterator& operator ++();
     bool operator!=(const iterator &other);
     void insert(value_type);
public:
 
   // Intentionally evil.
   dependent<ElemType> operator*();
 
   void foo();
   void constFoo() const;
 };
 
+template<typename ElemType>
+unsigned size(const dependent<ElemType>&);
+template<typename ElemType>
+unsigned length(const dependent<ElemType>&);
+
 template<typename First, typename Second>
 class doublyDependent{
  public:
   struct Value {
     First first;
     Second second;
   };
 

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

 // RUN: %check_clang_tidy %s modernize-loop-convert %t -- -- -I %S/Inputs/loop-convert
 
 #include "structures.h"
 
 namespace Array {
 
 const int N = 6;
 const int NMinusOne = N - 1;
void f() {
   // CHECK-FIXES-NEXT: printf("%d\n", (*I).X);
 
   for (dependent<Val *>::iterator I = Dpp.begin(), E = Dpp.end(); I != E; ++I) {
     printf("%d\n", (*I)->X);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & I : Dpp)
   // CHECK-FIXES-NEXT: printf("%d\n", I->X);
+
+  for (S::iterator It = begin(Ss), E = end(Ss); It != E; ++It) {
+    printf("s has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & It : Ss)
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", It.X);
+
+  for (S::iterator It = begin(*Ps), E = end(*Ps); It != E; ++It) {
+    printf("s has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & It : *Ps)
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", It.X);
+
+  for (S::iterator It = begin(*Ps); It != end(*Ps); ++It) {
+    printf("s has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & It : *Ps)
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", It.X);
+
+  for (S::const_iterator It = cbegin(Ss), E = cend(Ss); It != E; ++It) {
+    printf("s has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto It : Ss)
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", It.X);
 }
 
 // Tests to verify the proper use of auto where the init variable type and the
 // initializer type differ or are mostly the same except for const qualifiers.
 void different_type() {
   // Ss.begin() returns a type 'iterator' which is just a non-const pointer and
   // differs from const_iterator only on the const qualification.
   S Ss;
   for (S::const_iterator It = Ss.begin(), E = Ss.end(); It != E; ++It) {
     printf("s has value %d\n", (*It).X);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto It : Ss)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", It.X);
void f() {
   for (int I = 0, E = V.size(); E != I; ++I) {
     printf("Fibonacci number is %d\n", V[I]);
     Sum += V[I] + 2;
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (int I : V)
   // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", I);
   // CHECK-FIXES-NEXT: Sum += I + 2;
+
+  for (int I = 0, E = size(V); E != I; ++I) {
+    printf("Fibonacci number is %d\n", V[I]);
+    Sum += V[I] + 2;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", I);
+  // CHECK-FIXES-NEXT: Sum += I + 2;
+
+  for (int I = 0, E = size(V); E != I; ++I) {
+    V[I] = 0;
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & I : V)
+  // CHECK-FIXES-NEXT: I = 0;
+
+  // Although 'length' might be a valid free function, only std::size() is standardized
+  for (int I = 0, E = length(V); E != I; ++I) {
+    printf("Fibonacci number is %d\n", V[I]);
+    Sum += V[I] + 2;
+  }
+
+  dependent<Val> Vals;
+  for (int I = 0, E = size(Vals); E != I; ++I) {
+    Sum += Vals[I].X;
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & Val : Vals)
+  // CHECK-FIXES-NEXT: Sum += Val.X;
 }
 
 // Ensure that 'const auto &' is used with containers of non-trivial types.
 void constness() {
   int Sum = 0;
   for (int I = 0, E = Constv.size(); I < E; ++I) {
     printf("Fibonacci number is %d\n", Constv[I].X);
     Sum += Constv[I].X + 2;
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (const auto & I : Constv)
   // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", I.X);
   // CHECK-FIXES-NEXT: Sum += I.X + 2;
 
   for (int I = 0, E = Constv.size(); I < E; ++I) {