diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h +++ b/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; 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 @@ -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(ContainerMemberExpr); + assert(BeginMember); + QualType RetType = cast(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(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 TestDerefReturnsByValue = + ast_matchers::internal::Matcher 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 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(digThroughConstructors(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; -} - -/// 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(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(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(EndCallName); - if (!EndCall || !dyn_cast(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(InitVarName); const auto *EndVar = Nodes.getNodeAs(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(EndCallName); - const auto *BoundExpr = Nodes.getNodeAs(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(EndCall->getCallee())->isArrow(); - } - // We must know the container or an array length bound. - if (!ContainerExpr && !BoundExpr) - return; + if (FixerKind == LFK_Array) { + BoundExpr = Nodes.getNodeAs(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(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, 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 @@ -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::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.