diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include using namespace clang::ast_matchers; @@ -129,6 +130,8 @@ /// 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) @@ -136,7 +139,8 @@ /// 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. @@ -149,8 +153,10 @@ 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 = @@ -163,8 +169,10 @@ 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( @@ -223,6 +231,7 @@ /// \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) @@ -230,7 +239,8 @@ /// 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. @@ -265,10 +275,12 @@ hasMethod(hasName("end"))))))))) // qualType )); - StatementMatcher SizeCallMatcher = cxxMemberCallExpr( - argumentCountIs(0), callee(cxxMethodDecl(hasAnyName("size", "length"))), - on(anyOf(hasType(pointsTo(RecordWithBeginEnd)), - hasType(RecordWithBeginEnd)))); + StatementMatcher SizeCallMatcher = expr(anyOf( + cxxMemberCallExpr(argumentCountIs(0), + callee(cxxMethodDecl(hasAnyName("size", "length"))), + on(anyOf(hasType(pointsTo(RecordWithBeginEnd)), + hasType(RecordWithBeginEnd)))), + callExpr(argumentCountIs(1), callee(functionDecl(hasAnyName("size")))))); StatementMatcher EndInitMatcher = expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)), @@ -296,6 +308,35 @@ .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 +getContainerExpr(const Expr *Call) { + const Expr *Dug = digThroughConstructorsConversions(Call); + + if (const auto *TheCall = dyn_cast_or_null(Dug)) { + const auto *Member = dyn_cast(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(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 @@ -304,28 +345,20 @@ 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( - digThroughConstructorsConversions(Init)); - if (!TheCall || TheCall->getNumArgs() != 0) - return nullptr; - const auto *Member = dyn_cast(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 @@ -692,7 +725,7 @@ } 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(ContainerExpr)) + if (const auto *E = dyn_cast(ContainerExpr)) if (E->getOperator() != OO_Subscript) ContainerExpr = E->getArg(0); ContainerString = @@ -817,10 +850,10 @@ QualType InitVarType = InitVar->getType(); QualType CanonicalInitVarType = InitVarType.getCanonicalType(); - const auto *BeginCall = Nodes.getNodeAs(BeginCallName); + const auto *BeginCall = Nodes.getNodeAs(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 @@ -831,10 +864,15 @@ return false; } } else if (FixerKind == LFK_PseudoArray) { - // This call is required to obtain the container. - const auto *EndCall = Nodes.getNodeAs(EndCallName); - if (!EndCall || !isa(EndCall->getCallee())) + if (const auto *EndCall = Nodes.getNodeAs(EndCallName)) { + // This call is required to obtain the container. + if (!isa(EndCall->getCallee())) + return false; + } else if (Nodes.getNodeAs(EndCallName)) { + return true; + } else { return false; + } } return true; } @@ -873,7 +911,7 @@ // 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(EndCallName); + const auto *EndCall = Nodes.getNodeAs(EndCallName); const auto *BoundExpr = Nodes.getNodeAs(ConditionBoundName); // Find container expression of iterators and pseudoarrays, and determine if @@ -887,9 +925,8 @@ &Descriptor.ContainerNeedsDereference, /*IsReverse=*/FixerKind == LFK_ReverseIterator); } else if (FixerKind == LFK_PseudoArray) { - ContainerExpr = EndCall->getImplicitObjectArgument(); - Descriptor.ContainerNeedsDereference = - dyn_cast(EndCall->getCallee())->isArrow(); + std::tie(ContainerExpr, std::ignore, Descriptor.ContainerNeedsDereference) = + getContainerExpr(EndCall); } // We must know the container or an array length bound. diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -181,6 +181,10 @@ to check for container functions ``begin``/``end`` etc on base classes of the container type, instead of only as direct members of the container type itself. +- Improved :doc:`modernize-loop-convert ` to + refactor container based for loops that initialize iterators with free function calls + to ``begin``, ``end``, or ``size``. + - Improved :doc:`modernize-use-emplace ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst @@ -91,10 +91,18 @@ for (vector::iterator it = v.begin(); it != v.end(); ++it) cout << *it; + // reasonable conversion + for (vector::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++ diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h @@ -39,6 +39,14 @@ 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 { @@ -126,6 +134,11 @@ void constFoo() const; }; +template +unsigned size(const dependent&); +template +unsigned length(const dependent&); + template class dependent_derived : public dependent { }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp @@ -445,6 +445,34 @@ // 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 @@ -663,6 +691,36 @@ // 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; + + // 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 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.