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 @@ -17,11 +17,13 @@ #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" +#include "llvm/ADT/StringSet.h" #include "llvm/Support/Casting.h" #include "llvm/Support/raw_ostream.h" #include #include #include +#include #include using namespace clang::ast_matchers; @@ -66,6 +68,15 @@ static const char EndVarName[] = "endVar"; static const char DerefByValueResultName[] = "derefByValueResult"; static const char DerefByRefResultName[] = "derefByRefResult"; +static const llvm::StringSet<> MemberNames{"begin", "cbegin", "rbegin", + "crbegin", "end", "cend", + "rend", "crend", "size"}; +static const llvm::StringSet<> ADLNames{"begin", "cbegin", "rbegin", + "crbegin", "end", "cend", + "rend", "crend", "size"}; +static const llvm::StringSet<> 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 +140,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 +152,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 +161,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 +189,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 +253,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 +261,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 +297,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(hasName("size"))), + usesADL()), + callExpr(argumentCountIs(1), + callee(functionDecl(hasName("::std::size")))))); StatementMatcher EndInitMatcher = expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)), @@ -296,36 +333,97 @@ .bind(LoopNamePseudoArray); } +enum class IteratorCallKind { + ICK_Member, + ICK_ADL, + ICK_Std, +}; + +struct ContainerCall { + const Expr *Container; + StringRef Name; + bool IsArrow; + IteratorCallKind CallKind; +}; + +// 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 +static std::optional getContainerExpr(const Expr *Call) { + const Expr *Dug = digThroughConstructorsConversions(Call); + + IteratorCallKind CallKind = IteratorCallKind::ICK_Member; + + if (const auto *TheCall = dyn_cast_or_null(Dug)) { + CallKind = IteratorCallKind::ICK_Member; + if (const auto *Member = dyn_cast(TheCall->getCallee())) { + if (Member->getMemberDecl() == nullptr || + !MemberNames.contains(Member->getMemberDecl()->getName())) + return std::nullopt; + return ContainerCall{TheCall->getImplicitObjectArgument(), + Member->getMemberDecl()->getName(), + Member->isArrow(), CallKind}; + } else { + if (TheCall->getDirectCallee() == nullptr || + !MemberNames.contains(TheCall->getDirectCallee()->getName())) + return std::nullopt; + return ContainerCall{TheCall->getArg(0), + TheCall->getDirectCallee()->getName(), false, + CallKind}; + } + } else if (const auto *TheCall = dyn_cast_or_null(Dug)) { + if (TheCall->getNumArgs() != 1) + return std::nullopt; + + if (TheCall->usesADL()) { + if (TheCall->getDirectCallee() == nullptr || + !ADLNames.contains(TheCall->getDirectCallee()->getName())) + return std::nullopt; + CallKind = IteratorCallKind::ICK_ADL; + } else { + if (!StdNames.contains( + TheCall->getDirectCallee()->getQualifiedNameAsString())) + return std::nullopt; + CallKind = IteratorCallKind::ICK_Std; + } + + if (TheCall->getDirectCallee() == nullptr) + return std::nullopt; + + return ContainerCall{TheCall->getArg(0), + TheCall->getDirectCallee()->getName(), false, + CallKind}; + } + return std::nullopt; +} + /// 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::pair +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(); - 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; + std::optional Call = getContainerExpr(Init); + if (!Call) + return {}; + + *IsArrow = Call->IsArrow; + if (!Call->Name.consume_back(IsBegin ? "begin" : "end")) + return {}; + if (IsReverse && !Call->Name.consume_back("r")) + return {}; + if (!Call->Name.empty() && !Call->Name.equals("c")) + return {}; + return std::make_pair(Call->Container, Call->CallKind); } /// Determines the container whose begin() and end() functions are called @@ -341,13 +439,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 || @@ -832,10 +933,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 @@ -846,10 +947,12 @@ 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())) - return false; + if (const auto *EndCall = Nodes.getNodeAs(EndCallName)) { + // This call is required to obtain the container. + if (!isa(EndCall->getCallee())) + return false; + } + return Nodes.getNodeAs(EndCallName) != nullptr; } return true; } @@ -888,7 +991,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 @@ -902,9 +1005,11 @@ &Descriptor.ContainerNeedsDereference, /*IsReverse=*/FixerKind == LFK_ReverseIterator); } else if (FixerKind == LFK_PseudoArray) { - ContainerExpr = EndCall->getImplicitObjectArgument(); - Descriptor.ContainerNeedsDereference = - dyn_cast(EndCall->getCallee())->isArrow(); + std::optional Call = getContainerExpr(EndCall); + if (Call) { + ContainerExpr = Call->Container; + Descriptor.ContainerNeedsDereference = Call->IsArrow; + } } // 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 @@ -172,6 +172,10 @@ ` check to provide fixes for ``inline`` namespaces in the same format as :program:`clang-format`. +- Improved :doc:`modernize-loop-convert + ` to support for-loops with + iterators initialized by free functions like ``begin``, ``end``, or ``size``. + Removed checks ^^^^^^^^^^^^^^ 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. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-reverse.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-reverse.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-reverse.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-reverse.cpp @@ -40,6 +40,8 @@ // Make sure no header is included in this example // CHECK-FIXES-CUSTOM-NO-HEADER-NOT: #include +namespace ADL { + template struct Reversable { using iterator = T *; @@ -61,6 +63,28 @@ const_iterator crend() const; }; +template +constexpr auto rbegin(C& c) -> decltype(c.rbegin()) { return c.rbegin(); } +template +constexpr auto rend(C& c) -> decltype(c.rend()) { return c.rend(); } +template +constexpr auto rbegin(const C& c) -> decltype(c.rbegin()) { return c.rbegin(); } +template +constexpr auto rend(const C& c) -> decltype(c.rend()) { return c.rend(); } + +template +constexpr auto crbegin(C& c) -> decltype(c.crbegin()); +template +constexpr auto crend(C& c) -> decltype(c.crend()); +template +constexpr auto crbegin(const C& c) -> decltype(c.crbegin()); +template +constexpr auto crend(const C& c) -> decltype(c.crend()); + +} // namespace ADL + +using ADL::Reversable; + template void observe(const T &); template @@ -85,6 +109,24 @@ // CHECK-FIXES-NEXT: observe(Number); // CHECK-FIXES-NEXT: } + for (auto I = rbegin(Numbers), E = rend(Numbers); I != E; ++I) { + observe(*I); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES-RANGES: for (int Number : std::ranges::reverse_view(Numbers)) { + // CHECK-FIXES-CUSTOM: for (int Number : llvm::reverse(Numbers)) { + // CHECK-FIXES-NEXT: observe(Number); + // CHECK-FIXES-NEXT: } + + for (auto I = crbegin(Numbers), E = crend(Numbers); I != E; ++I) { + observe(*I); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES-RANGES: for (int Number : std::ranges::reverse_view(Numbers)) { + // CHECK-FIXES-CUSTOM: for (int Number : llvm::reverse(Numbers)) { + // CHECK-FIXES-NEXT: observe(Number); + // CHECK-FIXES-NEXT: } + // Ensure these bad loops aren't transformed. for (auto I = Numbers.rbegin(), E = Numbers.end(); I != E; ++I) { observe(*I); @@ -112,4 +154,13 @@ // CHECK-FIXES-CUSTOM: for (int Number : llvm::reverse(Numbers)) { // CHECK-FIXES-NEXT: observe(Number); // CHECK-FIXES-NEXT: } + + for (auto I = rbegin(Numbers), E = rend(Numbers); I != E; ++I) { + mutate(*I); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES-RANGES: for (int & Number : std::ranges::reverse_view(Numbers)) { + // CHECK-FIXES-CUSTOM: for (int & Number : llvm::reverse(Numbers)) { + // CHECK-FIXES-NEXT: mutate(Number); + // CHECK-FIXES-NEXT: } }