[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

265

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

[ccotter]

• alpha

[ccotter]

clang-tools-extra/docs/ReleaseNotes.rst

265

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

340

Remove const, it's unused.

346

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

173–174

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

192

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

302

Consider exclude single parameter methods.

920

`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

340

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

192

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

920

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

142

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

920

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

173–174

I think usesADL() accomplishes the same thing here.

[ccotter]

bump - any other feedback?

[PiotrZSL]

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

74

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

302

hasAnyName -> hasName, there is only 1 argument.

305

hasAnyName -> hasName, there is only 1 argument.

346

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

354

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

363

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

368

count -> contains

372

count -> contains

390–392

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

clang-tools-extra/docs/ReleaseNotes.rst

201–204

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 ?

737

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

333

if we can, please use enum class

363

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

927

no need for else after return

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

484

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

484

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 <optional>
+#include <tuple>
+#include <unordered_set>
 #include <utility>
 
 using namespace clang::ast_matchers;
 using namespace llvm;
 
 namespace clang::tidy {
 
 template <> struct OptionEnumMapping<modernize::Confidence::Level> {

 static const char LoopNamePseudoArray[] = "forLoopPseudoArray";
 static const char ConditionBoundName[] = "conditionBound";
 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 std::set<llvm::StringRef> ADLNames{"begin",   "cbegin", "rbegin",
+                                                "crbegin", "end",    "cend",
+                                                "rend",    "crend",  "size"};
+static const std::set<llvm::StringRef> StdNames{
+    "std::begin", "std::cbegin", "std::rbegin", "std::crbegin", "std::end",
+    "std::cend",  "std::rend",   "std::crend",  "std::size"};
 
 static const StatementMatcher integerComparisonMatcher() {
   return expr(ignoringParenImpCasts(
       declRefExpr(to(varDecl(equalsBoundNode(InitVarName))))));
 }
 
 static const DeclarationMatcher initToZeroMatcher() {
   return varDecl(

 /// 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) { ... }
+///   for (containerType::iterator it = std::begin(container),
+///        e = std::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)
+///   In the third/fourth examples:
+///     'end(container)' or 'std::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 BeginNameMatcherStd = IsReverse
+                                 ? hasAnyName("::std::rbegin", "::std::crbegin")
+                                 : hasAnyName("::std::begin", "::std::cbegin");
 
   auto EndNameMatcher =
       IsReverse ? hasAnyName("rend", "crend") : hasAnyName("end", "cend");
+  auto EndNameMatcherStd = IsReverse ? hasAnyName("::std::rend", "::std::crend")
+                                     : hasAnyName("::std::end", "::std::cend");
 
   StatementMatcher BeginCallMatcher =
-      cxxMemberCallExpr(argumentCountIs(0),
-                        callee(cxxMethodDecl(BeginNameMatcher)))
+      expr(anyOf(cxxMemberCallExpr(argumentCountIs(0),
+                                   callee(cxxMethodDecl(BeginNameMatcher))),
+                 callExpr(argumentCountIs(1),
+                          callee(functionDecl(BeginNameMatcher)), usesADL()),
+                 callExpr(argumentCountIs(1),
+                          callee(functionDecl(BeginNameMatcherStd)))))
           .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)),
+               usesADL()),
+      callExpr(argumentCountIs(1), callee(functionDecl(EndNameMatcherStd)))));
 
   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(
                                                       ))),     // qualType
       qualType(unless(isConstQualified()),
                hasUnqualifiedDesugaredType(recordType(hasDeclaration(
                    cxxRecordDecl(isSameOrDerivedFrom(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"))),
+               usesADL()),
+      callExpr(argumentCountIs(1),
+               callee(functionDecl(hasAnyName("::std::size"))))));
 
   StatementMatcher EndInitMatcher =
       expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)),
                  explicitCastExpr(hasSourceExpression(ignoringParenImpCasts(
                      expr(SizeCallMatcher).bind(EndCallName))))));
 
   DeclarationMatcher EndDeclMatcher =
       varDecl(hasInitializer(EndInitMatcher)).bind(EndVarName);
return forStmt(unless(isInTemplateInstantiation()),
                            declStmt(hasSingleDecl(initToZeroMatcher())))),
                  hasCondition(arrayConditionMatcher(IndexBoundMatcher)),
                  hasIncrement(
                      unaryOperator(hasOperatorName("++"),
                                    hasUnaryOperand(incrementVarMatcher()))))
       .bind(LoopNamePseudoArray);
 }
 
+enum IteratorCallKind {
+  ICK_Member,
+  ICK_ADL,
+  ICK_Std,
+};
+
+// 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 std::tuple<const Expr *, StringRef, bool, IteratorCallKind>
+getContainerExpr(const Expr *Call) {
+  const Expr *Dug = digThroughConstructorsConversions(Call);
+
+  IteratorCallKind CallKind;
+
+  if (const auto *TheCall = dyn_cast_or_null<CXXMemberCallExpr>(Dug)) {
+    CallKind = ICK_Member;
+    if (const auto *Member = dyn_cast<MemberExpr>(TheCall->getCallee())) {
+      return std::make_tuple(TheCall->getImplicitObjectArgument(),
+                             Member->getMemberDecl()->getName(),
+                             Member->isArrow(), CallKind);
+    } else {
+      return std::make_tuple(TheCall->getArg(0),
+                             TheCall->getDirectCallee()->getName(), false,
+                             CallKind);
+    }
+  } else if (const auto *TheCall = dyn_cast_or_null<CallExpr>(Dug)) {
+    if (TheCall->getNumArgs() != 1)
+      return std::make_tuple(nullptr, StringRef{}, false, CallKind);
+
+    if (TheCall->usesADL()) {
+      if (!ADLNames.count(TheCall->getDirectCallee()->getName()))
+        return {};
+      CallKind = ICK_ADL;
+    } else {
+      if (!StdNames.count(
+              TheCall->getDirectCallee()->getQualifiedNameAsString()))
+        return {};
+      CallKind = ICK_Std;
+    }
+
+    return std::make_tuple(TheCall->getArg(0),
+                           TheCall->getDirectCallee()->getName(), false,
+                           CallKind);
+  }
+  return {};
+}
+
 /// 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) {
+static std::tuple<const Expr *, IteratorCallKind>
+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)
-    return nullptr;
-  StringRef Name = Member->getMemberDecl()->getName();
+  StringRef Name;
+  const Expr *ContainerExpr;
+  IteratorCallKind CallKind;
+  std::tie(ContainerExpr, Name, *IsArrow, CallKind) = getContainerExpr(Init);
+  if (!ContainerExpr)
+    return {};
   if (!Name.consume_back(IsBegin ? "begin" : "end"))
-    return nullptr;
+    return {};
   if (IsReverse && !Name.consume_back("r"))
-    return nullptr;
+    return {};
   if (!Name.empty() && !Name.equals("c"))
-    return nullptr;
-
-  const Expr *SourceExpr = Member->getBase();
-  if (!SourceExpr)
-    return nullptr;
-
-  *IsArrow = Member->isArrow();
-  return SourceExpr;
+    return {};
+  return std::make_tuple(ContainerExpr, CallKind);
 }
 
 /// 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(
+  auto [BeginContainerExpr, BeginCallKind] = getContainerFromBeginEndCall(
       BeginExpr, /*IsBegin=*/true, &BeginIsArrow, IsReverse);
   if (!BeginContainerExpr)
     return nullptr;
 
-  const Expr *EndContainerExpr = getContainerFromBeginEndCall(
+  auto [EndContainerExpr, EndCallKind] = getContainerFromBeginEndCall(
       EndExpr, /*IsBegin=*/false, &EndIsArrow, IsReverse);
+  if (BeginCallKind != EndCallKind)
+    return nullptr;
+
   // 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;
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();
+    IteratorCallKind IgnoreCallKind;
+    std::tie(ContainerExpr, std::ignore, Descriptor.ContainerNeedsDereference,
+             IgnoreCallKind) = 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

- Deprecated check-local options `HeaderFileExtensions`
   <clang-tidy/checks/misc/definitions-in-headers>` check.
   Global options of the same name should be used instead.
 
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`misc-unused-using-decls
   <clang-tidy/checks/misc/unused-using-decls>` check.
   Global options of the same name should be used instead.
 
+- 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``.
+
 - In :doc:`modernize-use-default-member-init
   <clang-tidy/checks/modernize/use-default-member-init>` count template
   constructors toward hand written constructors so that they are skipped if more
   than one exists.
 
 - Fixed false positive in :doc:`modernize-use-equals-default
   <clang-tidy/checks/modernize/use-equals-default>` check for special member
   functions containing macros or preprocessor directives, and out-of-line special
- Fixed an issue in :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` when specifying an empty
   string for ``Prefix`` or ``Suffix`` options could result in the style not
   being used.
 
 - Fixed an issue in :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name>` when using
   ``DISABLED_`` in the test suite name.
 
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer

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 (vector<int>::iterator it = std::begin(v); it != std::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
 
+namespace std {
+template <class T> constexpr auto begin(T& t) -> decltype(t.begin());
+template <class T> constexpr auto begin(const T& t) -> decltype(t.begin());
+template <class T> constexpr auto end(T& t) -> decltype(t.end());
+template <class T> constexpr auto end(const T& t) -> decltype(t.end());
+template <class T> constexpr auto size(const T& t) -> decltype(t.size());
+} // namespace std
+
 extern "C" {
 extern int printf(const char *restrict, ...);
 }
 
 struct Val {int X; void g(); };
 
 struct MutableVal {
   void constFun(int) const;
struct NonTriviallyCopyable {
   int X;
 };
 
 struct TriviallyCopyableButBig {
   int X;
   char Array[16];
 };
 
+namespace ADT {
+
 struct S {
   typedef MutableVal *iterator;
   typedef const MutableVal *const_iterator;
   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::const_iterator end(const S&);
+S::const_iterator cbegin(const S&);
+S::const_iterator cend(const S&);
+S::iterator begin(S&);
+S::iterator end(S&);
+unsigned size(const 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);
     value_type X;
   };
   iterator begin();
   iterator end();
 };
+T::iterator begin(T&);
+T::iterator end(T&);
+
+} // namespace ADT
+
+using ADT::S;
+using ADT::T;
 
 struct Q {
   typedef int value_type;
   struct const_iterator {
     value_type &operator*();
     const value_type &operator*() const;
     const_iterator &operator++();
     bool operator!=(const const_iterator &other);

 
 struct X {
   S Ss;
   T Tt;
   U Uu;
   S getS();
 };
 
+namespace ADT {
+
 template<typename ElemType>
 class dependent {
  public:
   dependent<ElemType>();
   struct iterator_base {
     const ElemType& operator*()const;
     iterator_base& operator ++();
     bool operator!=(const iterator_base &other) const;
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 ElemType>
 class dependent_derived : public dependent<ElemType> {
 };
 
+} // namespace ADT
+
+using ADT::dependent;
+using ADT::dependent_derived;
+
 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("s0 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("s0 has value %d\n", It.X);
+
+  for (S::iterator It = std::begin(Ss), E = std::end(Ss); It != E; ++It) {
+    printf("s1 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("s1 has value %d\n", It.X);
+
+  for (S::iterator It = begin(*Ps), E = end(*Ps); It != E; ++It) {
+    printf("s2 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("s2 has value %d\n", It.X);
+
+  for (S::iterator It = begin(*Ps); It != end(*Ps); ++It) {
+    printf("s3 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("s3 has value %d\n", It.X);
+
+  for (S::const_iterator It = cbegin(Ss), E = cend(Ss); It != E; ++It) {
+    printf("s4 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("s4 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);
void f() {
   for (int I = 0, E = VD.size(); E != I; ++I) {
     printf("Fibonacci number is %d\n", VD[I]);
     Sum += VD[I] + 2;
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (int I : VD)
   // 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;
+
+  for (int I = 0, E = std::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 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;

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

 // RUN: %check_clang_tidy %s modernize-loop-convert %t -- -- -I %S/Inputs/loop-convert
 
 #include "structures.h"
 
 // CHECK-FIXES-NOT: for ({{.*[^:]:[^:].*}})
 // CHECK-MESSAGES-NOT: modernize-loop-convert
 
+namespace somenamespace {
+  template <class T> auto begin(T& t) -> decltype(t.begin());
+  template <class T> auto begin(const T& t) -> decltype(t.begin());
+  template <class T> auto end(T& t) -> decltype(t.end());
+  template <class T> auto end(const T& t) -> decltype(t.end());
+  template <class T> auto size(const T& t) -> decltype(t.size());
+} // namespace somenamespace
+
+struct SomeClass {
+  template <class T> static auto begin(T& t) -> decltype(t.begin());
+  template <class T> static auto begin(const T& t) -> decltype(t.begin());
+  template <class T> static auto end(T& t) -> decltype(t.end());
+  template <class T> static auto end(const T& t) -> decltype(t.end());
+  template <class T> static auto size(const T& t) -> decltype(t.size());
+};
+
 namespace Negative {
 
 const int N = 6;
 int Arr[N] = {1, 2, 3, 4, 5, 6};
 int (*pArr)[N] = &Arr;
 int Sum = 0;
 
 // Checks for the Index start and end:
for (int I = 0; I < N; ++I)
     Sum += Arr[I] + BadArr[I];
 
   for (int I = 0; I < N; ++I) {
     int K = BadArr[I];
     Sum += Arr[I] + K;
   }
 }
 
-}
+} // namespace Negative
 
 namespace NegativeIterator {
 
 S Ss;
 T Tt;
 U Tu;
 
 struct BadBeginEnd : T {
   iterator notBegin();
   iterator notEnd();
+  iterator begin(int);
+  iterator end(int);
+  iterator begin();
+  iterator end();
 };
 
 void notBeginOrEnd() {
   BadBeginEnd Bad;
   for (T::iterator I = Bad.notBegin(), E = Bad.end();  I != E; ++I)
     int K = *I;
 
   for (T::iterator I = Bad.begin(), E = Bad.notEnd();  I != E; ++I)
     int K = *I;
+
+  for (T::iterator I = Bad.begin(0), E = Bad.end(0);  I != E; ++I)
+    int K = *I;
 }
 
 void badLoopShapes() {
   for (T::iterator I = Tt.begin(), E = Tt.end(), F = E;  I != E; ++I)
     int K = *I;
 
   for (T::iterator I = Tt.begin(), E = Tt.end();  I != E;)
     int K = *I;
void iteratorOperatorCalled() {
   for (S::iterator I = Ss.begin(), E = Ss.end();  I != E; ++I)
     MutableVal K = *(++I);
 }
 
 void differentContainers() {
   T Other;
   for (T::iterator I = Tt.begin(), E = Other.end();  I != E; ++I)
     int K = *I;
+  for (T::iterator I = begin(Tt), E = end(Other);  I != E; ++I)
+    int K = *I;
 
   for (T::iterator I = Other.begin(), E = Tt.end();  I != E; ++I)
     int K = *I;
 
   S OtherS;
   for (S::iterator I = Ss.begin(), E = OtherS.end();  I != E; ++I)
     MutableVal K = *I;
 
   for (S::iterator I = OtherS.begin(), E = Ss.end();  I != E; ++I)
     MutableVal K = *I;
 }
 
+void mixedMemberAndADL() {
+  for (T::iterator I = Tt.begin(), E = end(Tt);  I != E; ++I)
+    int K = *I;
+  for (T::iterator I = begin(Tt), E = Tt.end();  I != E; ++I)
+    int K = *I;
+  for (T::iterator I = std::begin(Tt), E = Tt.end();  I != E; ++I)
+    int K = *I;
+  for (T::iterator I = std::begin(Tt), E = end(Tt);  I != E; ++I)
+    int K = *I;
+}
+
+void nonADLOrStdCalls() {
+  for (T::iterator I = SomeClass::begin(Tt), E = SomeClass::end(Tt);  I != E; ++I)
+    int K = *I;
+  for (T::iterator I = somenamespace::begin(Tt), E = somenamespace::end(Tt);  I != E; ++I)
+    int K = *I;
+}
+
 void wrongIterators() {
   T::iterator Other;
   for (T::iterator I = Tt.begin(), E = Tt.end(); I != Other; ++I)
     int K = *I;
 }
 
 struct EvilArrow : U {
   // Please, no one ever write code like this.

 }
 
 void wrongEnd() {
   int Bad;
   for (int I = 0, E = V.size(); I < Bad; ++I)
     Sum += V[I];
 }
 
+void nonADLOrStdCalls() {
+  for (int I = 0, E = somenamespace::size(V); E != I; ++I)
+    printf("Fibonacci number is %d\n", V[I]);
+  for (int I = 0, E = SomeClass::size(V); E != I; ++I)
+    printf("Fibonacci number is %d\n", V[I]);
+}
+
 // Checks to see that non-const member functions are not called on the container
 // object.
 // These could be conceivably allowed with a lower required confidence level.
 void memberFunctionCalled() {
   for (int I = 0; I < V.size(); ++I) {
     Sum += V[I];
     V.foo();
   }