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,21 +61,16 @@ static const char LoopNameReverseIterator[] = "forLoopReverseIterator"; static const char LoopNamePseudoArray[] = "forLoopPseudoArray"; static const char ConditionBoundName[] = "conditionBound"; -static const char ConditionVarName[] = "conditionVar"; -static const char IncrementVarName[] = "incrementVar"; static const char InitVarName[] = "initVar"; static const char BeginCallName[] = "beginCall"; static const char EndCallName[] = "endCall"; -static const char ConditionEndVarName[] = "conditionEndVar"; static const char EndVarName[] = "endVar"; static const char DerefByValueResultName[] = "derefByValueResult"; static const char DerefByRefResultName[] = "derefByRefResult"; -// shared matchers -static const TypeMatcher anyType() { return anything(); } static const StatementMatcher integerComparisonMatcher() { return expr(ignoringParenImpCasts( - declRefExpr(to(varDecl(hasType(isInteger())).bind(ConditionVarName))))); + declRefExpr(to(varDecl(equalsBoundNode(InitVarName)))))); } static const DeclarationMatcher initToZeroMatcher() { @@ -85,25 +80,19 @@ } static const StatementMatcher incrementVarMatcher() { - return declRefExpr(to(varDecl(hasType(isInteger())).bind(IncrementVarName))); + return declRefExpr(to(varDecl(equalsBoundNode(InitVarName)))); } /// The matcher for loops over arrays. -/// -/// In this general example, assuming 'j' and 'k' are of integral type: /// \code -/// for (int i = 0; j < 3 + 2; ++k) { ... } +/// for (int i = 0; i < 3 + 2; ++i) { ... } /// \endcode /// The following string identifiers are bound to these parts of the AST: -/// ConditionVarName: 'j' (as a VarDecl) /// ConditionBoundName: '3 + 2' (as an Expr) /// InitVarName: 'i' (as a VarDecl) -/// IncrementVarName: 'k' (as a VarDecl) /// LoopName: The entire for loop (as a ForStmt) /// /// Client code will need to make sure that: -/// - The three index variables identified by the matcher are the same -/// VarDecl. /// - The index variable is only used as an array index. /// - All arrays indexed by the loop are the same. StatementMatcher makeArrayLoopMatcher() { @@ -131,27 +120,22 @@ /// catch loops of the following textual forms (regardless of whether the /// iterator type is actually a pointer type or a class type): /// -/// Assuming f, g, and h are of type containerType::iterator, /// \code /// for (containerType::iterator it = container.begin(), -/// e = createIterator(); f != g; ++h) { ... } +/// e = createIterator(); it != e; ++it) { ... } /// for (containerType::iterator it = container.begin(); -/// f != anotherContainer.end(); ++h) { ... } +/// it != anotherContainer.end(); ++it) { ... } /// \endcode /// The following string identifiers are bound to the parts of the AST: /// InitVarName: 'it' (as a VarDecl) -/// ConditionVarName: 'f' (as a VarDecl) /// LoopName: The entire for loop (as a ForStmt) /// In the first example only: /// EndVarName: 'e' (as a VarDecl) -/// ConditionEndVarName: 'g' (as a VarDecl) /// In the second example only: /// EndCallName: 'container.end()' (as a CXXMemberCallExpr) /// /// Client code will need to make sure that: -/// - The iterator variables 'it', 'f', and 'h' are the same. /// - The two containers on which 'begin' and 'end' are called are the same. -/// - If the end iterator variable 'g' is defined, it is the same as 'f'. StatementMatcher makeIteratorLoopMatcher(bool IsReverse) { auto BeginNameMatcher = IsReverse ? hasAnyName("rbegin", "crbegin") @@ -180,13 +164,13 @@ StatementMatcher IteratorBoundMatcher = expr(anyOf(ignoringParenImpCasts( - declRefExpr(to(varDecl().bind(ConditionEndVarName)))), + declRefExpr(to(varDecl(equalsBoundNode(EndVarName))))), ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)), materializeTemporaryExpr(ignoringParenImpCasts( expr(EndCallMatcher).bind(EndCallName))))); - StatementMatcher IteratorComparisonMatcher = expr( - ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ConditionVarName))))); + StatementMatcher IteratorComparisonMatcher = expr(ignoringParenImpCasts( + declRefExpr(to(varDecl(equalsBoundNode(InitVarName)))))); // This matcher tests that a declaration is a CXXRecordDecl that has an // overloaded operator*(). If the operator*() returns by value instead of by @@ -217,13 +201,12 @@ hasIncrement(anyOf( unaryOperator(hasOperatorName("++"), hasUnaryOperand(declRefExpr( - to(varDecl(hasType(pointsTo(anyType()))) - .bind(IncrementVarName))))), + to(varDecl(equalsBoundNode(InitVarName)))))), cxxOperatorCallExpr( hasOverloadedOperatorName("++"), - hasArgument( - 0, declRefExpr(to(varDecl(TestDerefReturnsByValue) - .bind(IncrementVarName)))))))) + hasArgument(0, declRefExpr(to( + varDecl(equalsBoundNode(InitVarName), + TestDerefReturnsByValue)))))))) .bind(IsReverse ? LoopNameReverseIterator : LoopNameIterator); } @@ -233,27 +216,22 @@ /// loops of the following textual forms (regardless of whether the /// iterator type is actually a pointer type or a class type): /// -/// Assuming f, g, and h are of type containerType::iterator, /// \code -/// for (int i = 0, j = container.size(); f < g; ++h) { ... } -/// for (int i = 0; f < container.size(); ++h) { ... } +/// for (int i = 0, j = container.size(); i < j; ++i) { ... } +/// for (int i = 0; i < container.size(); ++i) { ... } /// \endcode /// The following string identifiers are bound to the parts of the AST: /// InitVarName: 'i' (as a VarDecl) -/// ConditionVarName: 'f' (as a VarDecl) /// LoopName: The entire for loop (as a ForStmt) /// In the first example only: /// EndVarName: 'j' (as a VarDecl) -/// ConditionEndVarName: 'g' (as a VarDecl) /// In the second example only: /// EndCallName: 'container.size()' (as a CXXMemberCallExpr) /// /// Client code will need to make sure that: -/// - The index variables 'i', 'f', and 'h' are the same. /// - The containers on which 'size()' is called is the container indexed. /// - The index variable is only used in overloaded operator[] or /// container.at(). -/// - If the end iterator variable 'g' is defined, it is the same as 'j'. /// - The container's iterators would not be invalidated during the loop. StatementMatcher makePseudoArrayLoopMatcher() { // Test that the incoming type has a record declaration that has methods @@ -296,8 +274,8 @@ varDecl(hasInitializer(EndInitMatcher)).bind(EndVarName); StatementMatcher IndexBoundMatcher = - expr(anyOf(ignoringParenImpCasts(declRefExpr(to( - varDecl(hasType(isInteger())).bind(ConditionEndVarName)))), + expr(anyOf(ignoringParenImpCasts( + declRefExpr(to(varDecl(equalsBoundNode(EndVarName))))), EndInitMatcher)); return forStmt( @@ -829,15 +807,7 @@ return false; // Check that we have exactly one index variable and at most one end variable. - const auto *LoopVar = Nodes.getNodeAs(IncrementVarName); - const auto *CondVar = Nodes.getNodeAs(ConditionVarName); const auto *InitVar = Nodes.getNodeAs(InitVarName); - if (!areSameVariable(LoopVar, CondVar) || !areSameVariable(LoopVar, InitVar)) - return false; - const auto *EndVar = Nodes.getNodeAs(EndVarName); - const auto *ConditionEndVar = Nodes.getNodeAs(ConditionEndVarName); - if (EndVar && !areSameVariable(EndVar, ConditionEndVar)) - return false; // FIXME: Try to put most of this logic inside a matcher. if (FixerKind == LFK_Iterator || FixerKind == LFK_ReverseIterator) { @@ -890,7 +860,7 @@ if (!isConvertible(Context, Nodes, Loop, FixerKind)) return; - const auto *LoopVar = Nodes.getNodeAs(IncrementVarName); + const auto *LoopVar = Nodes.getNodeAs(InitVarName); const auto *EndVar = Nodes.getNodeAs(EndVarName); // If the loop calls end()/size() after each iteration, lower our confidence