[clang-tidy][NFC] Use equalsBoundNode matchers to simplify LoopConvertCheck

[njames93]

Summary

Make use of the equalsBoundNode matcher to ensure Init, Conditon and Increment variables all refer to the same variable during matching.

[steveire]

Looks like a good simplification!

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

 
 namespace modernize {
 
 static const char LoopNameArray[] = "forLoopArray";
 static const char LoopNameIterator[] = "forLoopIterator";
 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() {
   return varDecl(
              hasInitializer(ignoringParenImpCasts(integerLiteral(equals(0)))))
       .bind(InitVarName);
 }
 
 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() {
   StatementMatcher ArrayBoundMatcher =
       expr(hasType(isInteger())).bind(ConditionBoundName);
 
   return forStmt(
              unless(isInTemplateInstantiation()),

 }
 
 /// The matcher used for iterator-based for loops.
 ///
 /// This matcher is more flexible than array-based loops. It will match
 /// 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")
                                     : hasAnyName("begin", "cbegin");
 
   auto EndNameMatcher =
       IsReverse ? hasAnyName("rend", "crend") : hasAnyName("end", "cend");
 
StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
   DeclarationMatcher EndDeclMatcher =
       varDecl(hasInitializer(anything())).bind(EndVarName);
 
   StatementMatcher EndCallMatcher = cxxMemberCallExpr(
       argumentCountIs(0), callee(cxxMethodDecl(EndNameMatcher)));
 
   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
   // reference then the return type is tagged with DerefByValueResultName.
   internal::Matcher<VarDecl> TestDerefReturnsByValue =
       hasType(hasUnqualifiedDesugaredType(
           recordType(hasDeclaration(cxxRecordDecl(hasMethod(cxxMethodDecl(
               hasOverloadedOperatorName("*"),
return forStmt(
                                         containsDeclaration(1, EndDeclMatcher)),
                                declStmt(hasSingleDecl(InitDeclMatcher)))),
              hasCondition(ignoringImplicit(binaryOperation(
                  hasOperatorName("!="), hasOperands(IteratorComparisonMatcher,
                                                     IteratorBoundMatcher)))),
              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);
 }
 
 /// The matcher used for array-like containers (pseudoarrays).
 ///
 /// 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):
 ///
-/// 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
   // called 'begin' and 'end'. If the incoming type is const, then make sure
   // these methods are also marked const.
   //
   // FIXME: To be completely thorough this matcher should also ensure the
   // return type of begin/end is an iterator that dereferences to the same as
StatementMatcher EndInitMatcher =
       expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)),
                  explicitCastExpr(hasSourceExpression(ignoringParenImpCasts(
                      expr(SizeCallMatcher).bind(EndCallName))))));
 
   DeclarationMatcher EndDeclMatcher =
       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(
              unless(isInTemplateInstantiation()),
              hasLoopInit(
                  anyOf(declStmt(declCountIs(2),
                                 containsDeclaration(0, initToZeroMatcher()),
                                 containsDeclaration(1, EndDeclMatcher)),
bool LoopConvertCheck::isConvertible(ASTContext *Context,
                                      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 *LoopVar = Nodes.getNodeAs<VarDecl>(IncrementVarName);
-  const auto *CondVar = Nodes.getNodeAs<VarDecl>(ConditionVarName);
   const auto *InitVar = Nodes.getNodeAs<VarDecl>(InitVarName);
-  if (!areSameVariable(LoopVar, CondVar) || !areSameVariable(LoopVar, InitVar))
-    return false;
-  const auto *EndVar = Nodes.getNodeAs<VarDecl>(EndVarName);
-  const auto *ConditionEndVar = Nodes.getNodeAs<VarDecl>(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) {
     QualType InitVarType = InitVar->getType();
     QualType CanonicalInitVarType = InitVarType.getCanonicalType();
 
     const auto *BeginCall = Nodes.getNodeAs<CXXMemberCallExpr>(BeginCallName);
     assert(BeginCall && "Bad Callback. No begin call expression");
if ((Loop = Nodes.getNodeAs<ForStmt>(LoopNameArray))) {
     Loop = Nodes.getNodeAs<ForStmt>(LoopNamePseudoArray);
     assert(Loop && "Bad Callback. No for statement");
     FixerKind = LFK_PseudoArray;
   }
 
   if (!isConvertible(Context, Nodes, Loop, FixerKind))
     return;
 
-  const auto *LoopVar = Nodes.getNodeAs<VarDecl>(IncrementVarName);
+  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