njames93 created this revision.
njames93 added reviewers: aaron.ballman, alexfh.
Herald added a subscriber: xazax.hun.
njames93 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Move all checking on the for loop declaration into the matchers.
Once a match is generated clients only need to examine the body of the for loop
to ensure its convertible.
This also brings the handling of match results for Iterator and PseudoArray
loops much more inline.
This supercedes D97889 <https://reviews.llvm.org/D97889>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D97955
Files:
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.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
@@ -447,6 +447,14 @@
// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
// CHECK-FIXES: for (int It : V)
// CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
+
+ for (dependent<int>::const_iterator It{V.begin()}, E = V.end();
+ It != E; ++It) {
+ printf("Fibonacci number is %d\n", *It);
+ }
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+ // CHEfCK-FIXES: for (int It : V)
+ // CHECfK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
}
// Tests to ensure that an implicit 'this' is picked up as the container.
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h
@@ -68,9 +68,6 @@
const UsageResult &Usages,
RangeDescriptor &Descriptor);
- bool isConvertible(ASTContext *Context, const ast_matchers::BoundNodes &Nodes,
- const ForStmt *Loop, LoopFixerKind FixerKind);
-
StringRef getReverseFunction() const;
StringRef getReverseHeader() const;
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
@@ -61,9 +61,8 @@
static const char LoopNameReverseIterator[] = "forLoopReverseIterator";
static const char LoopNamePseudoArray[] = "forLoopPseudoArray";
static const char ConditionBoundName[] = "conditionBound";
+static const char ContainerMemberExpr[] = "containerMemberExpr";
static const char InitVarName[] = "initVar";
-static const char BeginCallName[] = "beginCall";
-static const char EndCallName[] = "endCall";
static const char EndVarName[] = "endVar";
static const char DerefByValueResultName[] = "derefByValueResult";
static const char DerefByRefResultName[] = "derefByRefResult";
@@ -114,6 +113,59 @@
.bind(LoopNameArray);
}
+namespace {
+
+AST_MATCHER(ParmVarDecl, hasDefaultArg) {
+ return Node.getDefaultArg() != nullptr;
+}
+
+AST_MATCHER(VarDecl, isCStyleInit) {
+ return Node.getInitStyle() == VarDecl::CInit;
+}
+
+AST_MATCHER(VarDecl, samePtrTypeAsBeginResult) {
+ struct Predicate {
+ bool operator()(const ast_matchers::internal::BoundNodesMap &Nodes) const {
+ const auto *BeginMember =
+ Nodes.getNodeAs<MemberExpr>(ContainerMemberExpr);
+ assert(BeginMember);
+ QualType RetType = cast<CXXMethodDecl>(BeginMember->getMemberDecl())
+ ->getReturnType()
+ .getCanonicalType();
+ if (RetType->isPointerType())
+ return !Context.hasSameUnqualifiedType(VarType->getPointeeType(),
+ RetType->getPointeeType());
+ return false;
+ }
+
+ QualType VarType;
+ const ASTContext &Context;
+ };
+ QualType VarType = Node.getType().getCanonicalType();
+ if (!VarType->isPointerType())
+ return true;
+ return Builder->removeBindings(Predicate{VarType, Finder->getASTContext()});
+}
+
+AST_MATCHER(MemberExpr, matchesBoundBeginContainer) {
+ struct Predicate {
+ bool operator()(const ast_matchers::internal::BoundNodesMap &Nodes) const {
+ const auto *BeginMember =
+ Nodes.getNodeAs<MemberExpr>(ContainerMemberExpr);
+ assert(BeginMember);
+ if (BeginMember->isArrow() != Node.isArrow())
+ return true;
+
+ return !areSameExpr(Context, BeginMember->getBase(), Node.getBase());
+ }
+
+ const MemberExpr &Node;
+ ASTContext *Context;
+ };
+ return Builder->removeBindings(Predicate{Node, &Finder->getASTContext()});
+}
+} // namespace
+
/// The matcher used for iterator-based for loops.
///
/// This matcher is more flexible than array-based loops. It will match
@@ -122,20 +174,34 @@
///
/// \code
/// for (containerType::iterator it = container.begin(),
-/// e = createIterator(); it != e; ++it) { ... }
+/// e = container.end(); it != e; ++it) { ... }
/// for (containerType::iterator it = container.begin();
-/// it != anotherContainer.end(); ++it) { ... }
+/// it != container.end(); ++it) { ... }
/// \endcode
/// The following string identifiers are bound to the parts of the AST:
/// InitVarName: 'it' (as a VarDecl)
/// LoopName: The entire for loop (as a ForStmt)
+/// ContainerMemberExpr: 'container.begin' (as a MemberExpr)
/// In the first example only:
/// EndVarName: 'e' (as a VarDecl)
-/// In the second example only:
-/// EndCallName: 'container.end()' (as a CXXMemberCallExpr)
///
/// Client code will need to make sure that:
-/// - The two containers on which 'begin' and 'end' are called are the same.
+/// - 'it' is only accessed using the '*' or '->' operators.
+/// - The container's iterators would not be invalidated during the loop.
+///
+/// This will not match cases such as:
+/// \code
+/// for (containerType::iterator it = container.begin(),
+/// e = differentContainer.end(); it != e; ++it) { ... }
+/// for (containerType::iterator it = container.begin(),
+/// e = container.end(); it != f; ++it) { ... }
+/// for (containerType::iterator it = container.myBegin(),
+/// e = container.myEnd(); it != e; ++it) { ... }
+/// for (containerType::iterator it = container.begin(),
+/// e = container->end(); it != e; ++it) { ... }
+/// for (containerType::iterator it = container.begin(),
+/// e = container.end(); it != e; ++e) { ... }
+/// \endcode
StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
auto BeginNameMatcher = IsReverse ? hasAnyName("rbegin", "crbegin")
@@ -144,30 +210,40 @@
auto EndNameMatcher =
IsReverse ? hasAnyName("rend", "crend") : hasAnyName("end", "cend");
- StatementMatcher BeginCallMatcher =
- cxxMemberCallExpr(argumentCountIs(0),
- callee(cxxMethodDecl(BeginNameMatcher)))
- .bind(BeginCallName);
+ StatementMatcher BeginCallMatcher = cxxMemberCallExpr(
+ argumentCountIs(0),
+ callee(memberExpr(member(cxxMethodDecl(BeginNameMatcher)))
+ .bind(ContainerMemberExpr)));
+
+ auto DeclMatcher = [](auto MemberCallMatcher) {
+ // IgnoreUnlessSpelledInSource cant handle `Iterator
+ // It(getImplicitlyConvertableToIterator());` So Checking InitStyle does the
+ // job.
+ return varDecl(traverse(
+ TK_IgnoreUnlessSpelledInSource,
+ anyOf(
+ allOf(unless(isCStyleInit()),
+ hasInitializer(anyOf(cxxConstructExpr(argumentCountIs(1),
+ has(MemberCallMatcher)),
+ MemberCallMatcher))),
+ allOf(isCStyleInit(), hasInitializer(MemberCallMatcher)))));
+ };
DeclarationMatcher InitDeclMatcher =
- varDecl(hasInitializer(anyOf(ignoringParenImpCasts(BeginCallMatcher),
- materializeTemporaryExpr(
- ignoringParenImpCasts(BeginCallMatcher)),
- hasDescendant(BeginCallMatcher))))
+ varDecl(DeclMatcher(BeginCallMatcher), samePtrTypeAsBeginResult())
.bind(InitVarName);
- DeclarationMatcher EndDeclMatcher =
- varDecl(hasInitializer(anything())).bind(EndVarName);
-
- StatementMatcher EndCallMatcher = cxxMemberCallExpr(
- argumentCountIs(0), callee(cxxMethodDecl(EndNameMatcher)));
+ StatementMatcher EndCallMatcher =
+ cxxMemberCallExpr(argumentCountIs(0),
+ callee(memberExpr(member(cxxMethodDecl(EndNameMatcher)),
+ matchesBoundBeginContainer())));
+ DeclarationMatcher EndDeclMatcher =
+ DeclMatcher(EndCallMatcher).bind(EndVarName);
StatementMatcher IteratorBoundMatcher =
- expr(anyOf(ignoringParenImpCasts(
- declRefExpr(to(varDecl(equalsBoundNode(EndVarName))))),
- ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)),
- materializeTemporaryExpr(ignoringParenImpCasts(
- expr(EndCallMatcher).bind(EndCallName)))));
+ expr(traverse(TK_IgnoreUnlessSpelledInSource,
+ anyOf(declRefExpr(to(varDecl(equalsBoundNode(EndVarName)))),
+ expr(EndCallMatcher))));
StatementMatcher IteratorComparisonMatcher = expr(ignoringParenImpCasts(
declRefExpr(to(varDecl(equalsBoundNode(InitVarName))))));
@@ -175,7 +251,7 @@
// This matcher tests that a declaration is a CXXRecordDecl that has an
// overloaded operator*(). If the operator*() returns by value instead of by
// reference then the return type is tagged with DerefByValueResultName.
- internal::Matcher<VarDecl> TestDerefReturnsByValue =
+ ast_matchers::internal::Matcher<VarDecl> TestDerefReturnsByValue =
hasType(hasUnqualifiedDesugaredType(
recordType(hasDeclaration(cxxRecordDecl(hasMethod(cxxMethodDecl(
hasOverloadedOperatorName("*"),
@@ -214,25 +290,36 @@
///
/// This matcher is more flexible than array-based loops. It will match
/// loops of the following textual forms (regardless of whether the
-/// iterator type is actually a pointer type or a class type):
+/// iterator type is actually a pointer type or a class type).
+/// The container matched must have begin and end methods that either have no
+/// params or only default params. If the container is a const object, these
+/// methods must also be marked const:
///
/// \code
/// for (int i = 0, j = container.size(); i < j; ++i) { ... }
/// for (int i = 0; i < container.size(); ++i) { ... }
+/// for (int i = 0; i < container.length(); ++i) { ... }
/// \endcode
/// The following string identifiers are bound to the parts of the AST:
/// InitVarName: 'i' (as a VarDecl)
/// LoopName: The entire for loop (as a ForStmt)
-/// In the first example only:
+/// ContainerMemberExpr: 'container.size' or 'container.length' (as a
+/// MemberExpr) In the first example only:
/// EndVarName: 'j' (as a VarDecl)
-/// In the second example only:
-/// EndCallName: 'container.size()' (as a CXXMemberCallExpr)
///
/// Client code will need to make sure that:
/// - The containers on which 'size()' is called is the container indexed.
/// - The index variable is only used in overloaded operator[] or
/// container.at().
/// - The container's iterators would not be invalidated during the loop.
+///
+/// This will not match cases such as:
+/// \code
+/// for (int i = 0, j = container.size(); i < k; ++i) { ... }
+/// for (int i = 0, j = container.size(); i < j; ++j) { ... }
+/// for (int i = 0, j = container.size(); k < j; ++i) { ... }
+/// for (int i = 0, j = container.size(); i <= j; ++i) { ... }
+/// \endcode
StatementMatcher makePseudoArrayLoopMatcher() {
// Test that the incoming type has a record declaration that has methods
// called 'begin' and 'end'. If the incoming type is const, then make sure
@@ -246,29 +333,38 @@
// FIXME: Also, a record doesn't necessarily need begin() and end(). Free
// functions called begin() and end() taking the container as an argument
// are also allowed.
+
+ auto NoMandatoryParams =
+ anyOf(parameterCountIs(0), hasParameter(0, hasDefaultArg()));
+
TypeMatcher RecordWithBeginEnd = qualType(anyOf(
qualType(
isConstQualified(),
hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
- hasMethod(cxxMethodDecl(hasName("begin"), isConst())),
- hasMethod(cxxMethodDecl(hasName("end"),
+ hasMethod(cxxMethodDecl(NoMandatoryParams, hasName("begin"),
+ isConst())),
+ hasMethod(cxxMethodDecl(NoMandatoryParams, hasName("end"),
isConst())))) // hasDeclaration
))), // qualType
- qualType(unless(isConstQualified()),
- hasUnqualifiedDesugaredType(recordType(hasDeclaration(
- cxxRecordDecl(hasMethod(hasName("begin")),
- hasMethod(hasName("end"))))))) // qualType
+ qualType(
+ unless(isConstQualified()),
+ hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
+ hasMethod(allOf(NoMandatoryParams, hasName("begin"))),
+ hasMethod(
+ allOf(NoMandatoryParams, hasName("end")))))))) // qualType
));
StatementMatcher SizeCallMatcher = cxxMemberCallExpr(
- argumentCountIs(0), callee(cxxMethodDecl(hasAnyName("size", "length"))),
+ argumentCountIs(0),
+ callee(memberExpr(member(cxxMethodDecl(hasAnyName("size", "length"))))
+ .bind(ContainerMemberExpr)),
on(anyOf(hasType(pointsTo(RecordWithBeginEnd)),
hasType(RecordWithBeginEnd))));
StatementMatcher EndInitMatcher =
- expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)),
- explicitCastExpr(hasSourceExpression(ignoringParenImpCasts(
- expr(SizeCallMatcher).bind(EndCallName))))));
+ expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher)),
+ explicitCastExpr(hasSourceExpression(
+ ignoringParenImpCasts(expr(SizeCallMatcher))))));
DeclarationMatcher EndDeclMatcher =
varDecl(hasInitializer(EndInitMatcher)).bind(EndVarName);
@@ -297,68 +393,6 @@
.bind(LoopNamePseudoArray);
}
-/// 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) {
- // FIXME: Maybe allow declaration/initialization outside of the for loop.
- const auto *TheCall =
- dyn_cast_or_null<CXXMemberCallExpr>(digThroughConstructors(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;
-}
-
-/// Determines the container whose begin() and end() functions are called
-/// for an iterator-based loop.
-///
-/// BeginExpr must be a member call to a function named "begin()", and EndExpr
-/// must be a member.
-static const Expr *findContainer(ASTContext *Context, const Expr *BeginExpr,
- const Expr *EndExpr,
- bool *ContainerNeedsDereference,
- bool IsReverse) {
- // Now that we know the loop variable and test expression, make sure they are
- // valid.
- bool BeginIsArrow = false;
- bool EndIsArrow = false;
- const Expr *BeginContainerExpr = getContainerFromBeginEndCall(
- BeginExpr, /*IsBegin=*/true, &BeginIsArrow, IsReverse);
- if (!BeginContainerExpr)
- return nullptr;
-
- const Expr *EndContainerExpr = getContainerFromBeginEndCall(
- EndExpr, /*IsBegin=*/false, &EndIsArrow, IsReverse);
- // 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 ||
- !areSameExpr(Context, EndContainerExpr, BeginContainerExpr))
- return nullptr;
-
- *ContainerNeedsDereference = BeginIsArrow;
- return BeginContainerExpr;
-}
-
/// Obtain the original source code text from a SourceRange.
static StringRef getStringFromRange(SourceManager &SourceMgr,
const LangOptions &LangOpts,
@@ -795,47 +829,6 @@
getArrayLoopQualifiers(Context, Nodes, ContainerExpr, Usages, Descriptor);
}
-/// Check some of the conditions that must be met for the loop to be
-/// convertible.
-bool LoopConvertCheck::isConvertible(ASTContext *Context,
- const ast_matchers::BoundNodes &Nodes,
- const ForStmt *Loop,
- LoopFixerKind FixerKind) {
- // If we already modified the range of this for loop, don't do any further
- // updates on this iteration.
- if (TUInfo->getReplacedVars().count(Loop))
- return false;
-
- // Check that we have exactly one index variable and at most one end variable.
- const auto *InitVar = Nodes.getNodeAs<VarDecl>(InitVarName);
-
- // FIXME: Try to put most of this logic inside a matcher.
- if (FixerKind == LFK_Iterator || FixerKind == LFK_ReverseIterator) {
- QualType InitVarType = InitVar->getType();
- QualType CanonicalInitVarType = InitVarType.getCanonicalType();
-
- const auto *BeginCall = Nodes.getNodeAs<CXXMemberCallExpr>(BeginCallName);
- assert(BeginCall && "Bad Callback. No begin call expression");
- QualType CanonicalBeginType =
- BeginCall->getMethodDecl()->getReturnType().getCanonicalType();
- if (CanonicalBeginType->isPointerType() &&
- CanonicalInitVarType->isPointerType()) {
- // If the initializer and the variable are both pointers check if the
- // un-qualified pointee types match, otherwise we don't use auto.
- if (!Context->hasSameUnqualifiedType(
- CanonicalBeginType->getPointeeType(),
- CanonicalInitVarType->getPointeeType()))
- return false;
- }
- } else if (FixerKind == LFK_PseudoArray) {
- // This call is required to obtain the container.
- const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName);
- if (!EndCall || !dyn_cast<MemberExpr>(EndCall->getCallee()))
- return false;
- }
- return true;
-}
-
void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
const BoundNodes &Nodes = Result.Nodes;
Confidence ConfidenceLevel(Confidence::CL_Safe);
@@ -857,41 +850,41 @@
FixerKind = LFK_PseudoArray;
}
- if (!isConvertible(Context, Nodes, Loop, FixerKind))
+ // If we already modified the range of this for loop, don't do any further
+ // updates on this iteration.
+ if (TUInfo->getReplacedVars().count(Loop))
return;
const auto *LoopVar = Nodes.getNodeAs<VarDecl>(InitVarName);
- const auto *EndVar = Nodes.getNodeAs<VarDecl>(EndVarName);
-
- // If the loop calls end()/size() after each iteration, lower our confidence
- // level.
- if (FixerKind != LFK_Array && !EndVar)
- ConfidenceLevel.lowerTo(Confidence::CL_Reasonable);
+ const VarDecl *EndVar = Nodes.getNodeAs<VarDecl>(EndVarName);
// 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 *BoundExpr = Nodes.getNodeAs<Expr>(ConditionBoundName);
+ const Expr *BoundExpr = nullptr;
// Find container expression of iterators and pseudoarrays, and determine if
// this expression needs to be dereferenced to obtain the container.
// With array loops, the container is often discovered during the
// ForLoopIndexUseVisitor traversal.
const Expr *ContainerExpr = nullptr;
- if (FixerKind == LFK_Iterator || FixerKind == LFK_ReverseIterator) {
- ContainerExpr = findContainer(
- Context, LoopVar->getInit(), EndVar ? EndVar->getInit() : EndCall,
- &Descriptor.ContainerNeedsDereference,
- /*IsReverse=*/FixerKind == LFK_ReverseIterator);
- } else if (FixerKind == LFK_PseudoArray) {
- ContainerExpr = EndCall->getImplicitObjectArgument();
- Descriptor.ContainerNeedsDereference =
- dyn_cast<MemberExpr>(EndCall->getCallee())->isArrow();
- }
- // We must know the container or an array length bound.
- if (!ContainerExpr && !BoundExpr)
- return;
+ if (FixerKind == LFK_Array) {
+ BoundExpr = Nodes.getNodeAs<Expr>(ConditionBoundName);
+ assert(BoundExpr &&
+ "Bad callback, ConditionBoundName must be bounded in Array loops");
+ } else {
+ // If the loop calls end()/size() after each iteration, lower our confidence
+ // level.
+ if (!EndVar)
+ ConfidenceLevel.lowerTo(Confidence::CL_Reasonable);
+ const auto *CE = Result.Nodes.getNodeAs<MemberExpr>(ContainerMemberExpr);
+ assert(
+ CE &&
+ "Bad callback, ContainerMemberExpr must be bounded in Container loops");
+ assert(CE->getBase());
+ ContainerExpr = CE->getBase();
+ Descriptor.ContainerNeedsDereference = CE->isArrow();
+ }
ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
BoundExpr,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
