[clang-tools-extra] 6a1f8ef - [clang-tidy] Support begin/end free functions in modernize-loop-convert
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Sat Aug 5 14:08:27 PDT 2023
Author: Chris Cotter
Date: 2023-08-05T20:55:48Z
New Revision: 6a1f8ef8a7aaefea80ef0bc7c6c462a96215b50e
URL: https://github.com/llvm/llvm-project/commit/6a1f8ef8a7aaefea80ef0bc7c6c462a96215b50e
DIFF: https://github.com/llvm/llvm-project/commit/6a1f8ef8a7aaefea80ef0bc7c6c462a96215b50e.diff
LOG: [clang-tidy] Support begin/end free functions in modernize-loop-convert
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.
Reviewed By: PiotrZSL
Differential Revision: https://reviews.llvm.org/D140760
Added:
Modified:
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst
clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-negative.cpp
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-reverse.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 618d1860ff8bd0..dc3b0f70fb8add 100644
--- 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 <cassert>
#include <cstring>
#include <optional>
+#include <tuple>
#include <utility>
using namespace clang::ast_matchers;
@@ -66,6 +68,15 @@ static const char EndCallName[] = "endCall";
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 @@ StatementMatcher makeArrayLoopMatcher() {
/// 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 @@ StatementMatcher makeArrayLoopMatcher() {
/// 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 @@ 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 =
@@ -163,8 +189,12 @@ StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
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 @@ StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
/// \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 @@ StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
/// 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 @@ StatementMatcher makePseudoArrayLoopMatcher() {
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 @@ StatementMatcher makePseudoArrayLoopMatcher() {
.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<const Expr *, StringRef, bool, IteratorCallKind>
+static std::optional<ContainerCall> getContainerExpr(const Expr *Call) {
+ const Expr *Dug = digThroughConstructorsConversions(Call);
+
+ IteratorCallKind CallKind = IteratorCallKind::ICK_Member;
+
+ if (const auto *TheCall = dyn_cast_or_null<CXXMemberCallExpr>(Dug)) {
+ CallKind = IteratorCallKind::ICK_Member;
+ if (const auto *Member = dyn_cast<MemberExpr>(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<CallExpr>(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<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();
- 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<ContainerCall> 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 @@ static const Expr *findContainer(ASTContext *Context, const Expr *BeginExpr,
// 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 @@ bool LoopConvertCheck::isConvertible(ASTContext *Context,
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
@@ -846,10 +947,12 @@ bool LoopConvertCheck::isConvertible(ASTContext *Context,
return false;
}
} else if (FixerKind == LFK_PseudoArray) {
- // This call is required to obtain the container.
- const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName);
- if (!EndCall || !isa<MemberExpr>(EndCall->getCallee()))
- return false;
+ if (const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName)) {
+ // This call is required to obtain the container.
+ if (!isa<MemberExpr>(EndCall->getCallee()))
+ return false;
+ }
+ return Nodes.getNodeAs<CallExpr>(EndCallName) != nullptr;
}
return true;
}
@@ -888,7 +991,7 @@ void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
// 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
@@ -902,9 +1005,11 @@ void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
&Descriptor.ContainerNeedsDereference,
/*IsReverse=*/FixerKind == LFK_ReverseIterator);
} else if (FixerKind == LFK_PseudoArray) {
- ContainerExpr = EndCall->getImplicitObjectArgument();
- Descriptor.ContainerNeedsDereference =
- dyn_cast<MemberExpr>(EndCall->getCallee())->isArrow();
+ std::optional<ContainerCall> 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
index 59ec57b6db4ade..ba79e8df786135 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -172,6 +172,10 @@ Changes in existing checks
<clang-tidy/checks/llvm/namespace-comment>` check to provide fixes for
``inline`` namespaces in the same format as :program:`clang-format`.
+- Improved :doc:`modernize-loop-convert
+ <clang-tidy/checks/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
index a194392bc831bd..9aa655feb53389 100644
--- 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 @@ Original:
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++
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
index bc025ceab1f7a9..839511da306313 100644
--- 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 <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, ...);
}
@@ -28,6 +36,8 @@ struct TriviallyCopyableButBig {
char Array[16];
};
+namespace ADT {
+
struct S {
typedef MutableVal *iterator;
typedef const MutableVal *const_iterator;
@@ -39,6 +49,14 @@ struct S {
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 @@ struct T {
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 @@ struct X {
S getS();
};
+namespace ADT {
+
template<typename ElemType>
class dependent {
public:
@@ -126,10 +153,20 @@ class dependent {
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:
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
index 96b39563587871..71ae4c46e6a5e9 100644
--- 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 @@ void f() {
// 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 @@ void f() {
// 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.
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
index e6957403b77d5e..a6f95c06717167 100644
--- 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 <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;
@@ -92,7 +108,7 @@ void multipleArrays() {
}
}
-}
+} // namespace Negative
namespace NegativeIterator {
@@ -103,6 +119,10 @@ U Tu;
struct BadBeginEnd : T {
iterator notBegin();
iterator notEnd();
+ iterator begin(int);
+ iterator end(int);
+ iterator begin();
+ iterator end();
};
void notBeginOrEnd() {
@@ -112,6 +132,9 @@ void notBeginOrEnd() {
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 @@ void
diff erentContainers() {
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 @@ void
diff erentContainers() {
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 @@ void wrongEnd() {
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
index 63da130e74c626..15f35edbd9b35b 100644
--- 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 <typename T>
struct Reversable {
using iterator = T *;
@@ -61,6 +63,28 @@ struct Reversable {
const_iterator crend() const;
};
+template <typename C>
+constexpr auto rbegin(C& c) -> decltype(c.rbegin()) { return c.rbegin(); }
+template <typename C>
+constexpr auto rend(C& c) -> decltype(c.rend()) { return c.rend(); }
+template <typename C>
+constexpr auto rbegin(const C& c) -> decltype(c.rbegin()) { return c.rbegin(); }
+template <typename C>
+constexpr auto rend(const C& c) -> decltype(c.rend()) { return c.rend(); }
+
+template <typename C>
+constexpr auto crbegin(C& c) -> decltype(c.crbegin());
+template <typename C>
+constexpr auto crend(C& c) -> decltype(c.crend());
+template <typename C>
+constexpr auto crbegin(const C& c) -> decltype(c.crbegin());
+template <typename C>
+constexpr auto crend(const C& c) -> decltype(c.crend());
+
+} // namespace ADL
+
+using ADL::Reversable;
+
template <typename T>
void observe(const T &);
template <typename T>
@@ -85,6 +109,24 @@ void constContainer(const Reversable<int> &Numbers) {
// 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 @@ void nonConstContainer(Reversable<int> &Numbers) {
// 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: }
}
More information about the cfe-commits
mailing list