njames93 updated this revision to Diff 329765.
njames93 marked an inline comment as done.
njames93 added a comment.
Update.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97955/new/
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
@@ -474,6 +474,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";
@@ -119,6 +118,44 @@
.bind(LoopNameArray);
}
+namespace {
+AST_MATCHER(VarDecl, isCStyleInit) {
+ return Node.getInitStyle() == VarDecl::CInit;
+}
+
+AST_MATCHER(VarDecl, samePtrTypeAsBeginResult) {
+ QualType VarType = Node.getType().getCanonicalType();
+ if (!VarType->isPointerType())
+ return true;
+ return Builder->removeBindings(
+ [&](const ast_matchers::internal::BoundNodesMap &Nodes) {
+ const auto *BeginMember =
+ Nodes.getNodeAs<MemberExpr>(ContainerMemberExpr);
+ assert(BeginMember);
+ QualType RetType = cast<CXXMethodDecl>(BeginMember->getMemberDecl())
+ ->getReturnType()
+ .getCanonicalType();
+ if (RetType->isPointerType())
+ return !Finder->getASTContext().hasSameUnqualifiedType(
+ VarType->getPointeeType(), RetType->getPointeeType());
+ return false;
+ });
+}
+
+AST_MATCHER(MemberExpr, matchesBoundBeginContainer) {
+ return Builder->removeBindings(
+ [&](const ast_matchers::internal::BoundNodesMap &Nodes) {
+ const auto *BeginMember =
+ Nodes.getNodeAs<MemberExpr>(ContainerMemberExpr);
+ assert(BeginMember);
+ if (BeginMember->isArrow() != Node.isArrow())
+ return true;
+ return !areSameExpr(&Finder->getASTContext(), BeginMember->getBase(),
+ Node.getBase());
+ });
+}
+} // namespace
+
/// The matcher used for iterator-based for loops.
///
/// This matcher is more flexible than array-based loops. It will match
@@ -127,20 +164,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")
@@ -149,30 +200,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))))));
@@ -180,7 +241,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("*"),
@@ -219,25 +280,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
@@ -251,29 +323,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.
+
+ ast_matchers::internal::Matcher<CXXMethodDecl> NoMandatoryParams =
+ anyOf(parameterCountIs(0), hasParameter(0, hasInitializer(anything())));
+
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);
@@ -296,68 +377,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,
@@ -794,47 +813,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);
@@ -856,41 +834,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);
-
// 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
