ccotter updated this revision to Diff 490028.
ccotter marked an inline comment as done.
ccotter added a comment.
- fix merge
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140760/new/
https://reviews.llvm.org/D140760
Files:
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
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
+++ 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<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.
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
+++ 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<typename ElemType>
+unsigned size(const dependent<ElemType>&);
+template<typename ElemType>
+unsigned length(const dependent<ElemType>&);
+
template<typename ElemType>
class dependent_derived : public dependent<ElemType> {
};
Index: clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst
@@ -91,10 +91,18 @@
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 (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++
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ 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 <clang-tidy/checks/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 <clang-tidy/checks/modernize/use-emplace>`
check.
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -22,6 +22,7 @@
#include <cassert>
#include <cstring>
#include <optional>
+#include <tuple>
#include <utility>
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<const Expr *, StringRef, bool>
+getContainerExpr(const Expr *Call) {
+ const Expr *Dug = digThroughConstructorsConversions(Call);
+
+ if (const auto *TheCall = dyn_cast_or_null<CXXMemberCallExpr>(Dug)) {
+ const auto *Member = dyn_cast<MemberExpr>(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<CallExpr>(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<CXXMemberCallExpr>(
- digThroughConstructorsConversions(Init));
- if (!TheCall || TheCall->getNumArgs() != 0)
- return nullptr;
- const auto *Member = dyn_cast<MemberExpr>(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<CXXOperatorCallExpr>(ContainerExpr))
+ if (const auto *E = dyn_cast<CXXOperatorCallExpr>(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<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
@@ -831,10 +864,15 @@
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()))
+ if (const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName)) {
+ // This call is required to obtain the container.
+ if (!isa<MemberExpr>(EndCall->getCallee()))
+ return false;
+ } else if (Nodes.getNodeAs<CallExpr>(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<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
@@ -887,9 +925,8 @@
&Descriptor.ContainerNeedsDereference,
/*IsReverse=*/FixerKind == LFK_ReverseIterator);
} else if (FixerKind == LFK_PseudoArray) {
- ContainerExpr = EndCall->getImplicitObjectArgument();
- Descriptor.ContainerNeedsDereference =
- dyn_cast<MemberExpr>(EndCall->getCallee())->isArrow();
+ std::tie(ContainerExpr, std::ignore, Descriptor.ContainerNeedsDereference) =
+ getContainerExpr(EndCall);
}
// We must know the container or an array length bound.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
