njames93 created this revision.
njames93 added reviewers: alexfh, aaron.ballman, gribozavr2.
Herald added a subscriber: xazax.hun.
njames93 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Make use of the `equalsBoundNode` matcher to ensure Init, Conditon and
Increment variables all refer to the same variable during matching.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D97639
Files:
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
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,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<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) {
@@ -890,7 +860,7 @@
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
