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,8 @@ #include #include #include +#include +#include #include using namespace clang::ast_matchers; @@ -66,6 +68,12 @@ static const char EndVarName[] = "endVar"; static const char DerefByValueResultName[] = "derefByValueResult"; static const char DerefByRefResultName[] = "derefByRefResult"; +static const std::set ADLNames{"begin", "cbegin", "rbegin", + "crbegin", "end", "cend", + "rend", "crend", "size"}; +static const std::set 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( @@ -129,6 +137,10 @@ /// 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) @@ -137,6 +149,8 @@ /// 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. @@ -144,13 +158,22 @@ 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 = @@ -163,8 +186,12 @@ 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( @@ -223,6 +250,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 +258,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 +294,15 @@ 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"))), + usesADL()), + callExpr(argumentCountIs(1), + callee(functionDecl(hasAnyName("::std::size")))))); StatementMatcher EndInitMatcher = expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)), @@ -296,36 +330,81 @@ .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 +getContainerExpr(const Expr *Call) { + const Expr *Dug = digThroughConstructorsConversions(Call); + + IteratorCallKind CallKind; + + if (const auto *TheCall = dyn_cast_or_null(Dug)) { + CallKind = ICK_Member; + if (const auto *Member = dyn_cast(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(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 +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) - 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 @@ -341,13 +420,16 @@ // 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 || @@ -692,7 +774,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 +899,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 +913,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 +960,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 +974,9 @@ &Descriptor.ContainerNeedsDereference, /*IsReverse=*/FixerKind == LFK_ReverseIterator); } else if (FixerKind == LFK_PseudoArray) { - ContainerExpr = EndCall->getImplicitObjectArgument(); - Descriptor.ContainerNeedsDereference = - dyn_cast(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. 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 @@ -169,6 +169,11 @@ ` check. Global options of the same name should be used instead. +- Improved :doc:`modernize-loop-convert + ` to refactor container based + for loops that initialize iterators with free function calls to ``begin``, + ``end``, or ``size``. + - Fixed reading `HungarianNotation.CString.*` options in :doc:`readability-identifier-naming ` 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,22 @@ 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 (vector::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++ 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 @@ -1,6 +1,14 @@ #ifndef STRUCTURES_H #define STRUCTURES_H +namespace std { +template constexpr auto begin(T& t) -> decltype(t.begin()); +template constexpr auto begin(const T& t) -> decltype(t.begin()); +template constexpr auto end(T& t) -> decltype(t.end()); +template constexpr auto end(const T& t) -> decltype(t.end()); +template constexpr auto size(const T& t) -> decltype(t.size()); +} // namespace std + extern "C" { extern int printf(const char *restrict, ...); } @@ -28,6 +36,8 @@ char Array[16]; }; +namespace ADT { + struct S { typedef MutableVal *iterator; typedef const MutableVal *const_iterator; @@ -39,6 +49,14 @@ 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 { @@ -52,6 +70,13 @@ 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; @@ -90,6 +115,8 @@ S getS(); }; +namespace ADT { + template class dependent { public: @@ -126,10 +153,20 @@ void constFoo() const; }; +template +unsigned size(const dependent&); +template +unsigned length(const dependent&); + template class dependent_derived : public dependent { }; +} // namespace ADT + +using ADT::dependent; +using ADT::dependent_derived; + template class doublyDependent{ public: 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,41 @@ // 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 @@ -663,6 +698,43 @@ // 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 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. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-negative.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-negative.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-negative.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-negative.cpp @@ -5,6 +5,22 @@ // CHECK-FIXES-NOT: for ({{.*[^:]:[^:].*}}) // CHECK-MESSAGES-NOT: modernize-loop-convert +namespace somenamespace { + template auto begin(T& t) -> decltype(t.begin()); + template auto begin(const T& t) -> decltype(t.begin()); + template auto end(T& t) -> decltype(t.end()); + template auto end(const T& t) -> decltype(t.end()); + template auto size(const T& t) -> decltype(t.size()); +} // namespace somenamespace + +struct SomeClass { + template static auto begin(T& t) -> decltype(t.begin()); + template static auto begin(const T& t) -> decltype(t.begin()); + template static auto end(T& t) -> decltype(t.end()); + template static auto end(const T& t) -> decltype(t.end()); + template static auto size(const T& t) -> decltype(t.size()); +}; + namespace Negative { const int N = 6; @@ -92,7 +108,7 @@ } } -} +} // namespace Negative namespace NegativeIterator { @@ -103,6 +119,10 @@ struct BadBeginEnd : T { iterator notBegin(); iterator notEnd(); + iterator begin(int); + iterator end(int); + iterator begin(); + iterator end(); }; void notBeginOrEnd() { @@ -112,6 +132,9 @@ 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() { @@ -202,6 +225,8 @@ 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; @@ -214,6 +239,24 @@ 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) @@ -379,6 +422,13 @@ 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.