[clang-tidy] Use new mapping matchers

Use mapAnyOf() and matchers based on it.

Use of binaryOperation() means that modernize-loop-convert and
readability-container-size-empty can now be used with rewritten binary
operators.

Differential Revision: https://reviews.llvm.org/D94131
This commit is contained in:
Stephen Kelly
2021-01-02 21:18:29 +00:00
parent 1f06f41993
commit c0199b2a21
16 changed files with 173 additions and 119 deletions

View File

@@ -21,9 +21,9 @@ namespace bugprone {
static internal::Matcher<Stmt>
loopEndingStmt(internal::Matcher<Stmt> Internal) {
return stmt(anyOf(breakStmt(Internal), returnStmt(Internal),
gotoStmt(Internal), cxxThrowExpr(Internal),
callExpr(Internal, callee(functionDecl(isNoReturn())))));
return stmt(anyOf(
mapAnyOf(breakStmt, returnStmt, gotoStmt, cxxThrowExpr).with(Internal),
callExpr(Internal, callee(functionDecl(isNoReturn())))));
}
/// Return whether `Var` was changed in `LoopStmt`.
@@ -122,8 +122,8 @@ void InfiniteLoopCheck::registerMatchers(MatchFinder *Finder) {
unless(hasBody(hasDescendant(
loopEndingStmt(forFunction(equalsBoundNode("func")))))));
Finder->addMatcher(stmt(anyOf(whileStmt(LoopCondition), doStmt(LoopCondition),
forStmt(LoopCondition)))
Finder->addMatcher(mapAnyOf(whileStmt, doStmt, forStmt)
.with(LoopCondition)
.bind("loop-stmt"),
this);
}

View File

@@ -59,34 +59,20 @@ void SpuriouslyWakeUpFunctionsCheck::registerMatchers(MatchFinder *Finder) {
if (getLangOpts().CPlusPlus) {
// Check for `CON54-CPP`
Finder->addMatcher(
ifStmt(
allOf(HasWaitDescendantCpp,
unless(anyOf(hasDescendant(ifStmt(HasWaitDescendantCpp)),
hasDescendant(whileStmt(HasWaitDescendantCpp)),
hasDescendant(forStmt(HasWaitDescendantCpp)),
hasDescendant(doStmt(HasWaitDescendantCpp)))))
),
ifStmt(HasWaitDescendantCpp,
unless(hasDescendant(mapAnyOf(ifStmt, whileStmt, forStmt, doStmt)
.with(HasWaitDescendantCpp)))),
this);
} else {
// Check for `CON36-C`
Finder->addMatcher(
ifStmt(
allOf(HasWaitDescendantC,
unless(anyOf(hasDescendant(ifStmt(HasWaitDescendantC)),
hasDescendant(whileStmt(HasWaitDescendantC)),
hasDescendant(forStmt(HasWaitDescendantC)),
hasDescendant(doStmt(HasWaitDescendantC)),
hasParent(whileStmt()),
hasParent(compoundStmt(hasParent(whileStmt()))),
hasParent(forStmt()),
hasParent(compoundStmt(hasParent(forStmt()))),
hasParent(doStmt()),
hasParent(compoundStmt(hasParent(doStmt())))))
))
,
ifStmt(HasWaitDescendantC,
unless(anyOf(
hasDescendant(mapAnyOf(ifStmt, whileStmt, forStmt, doStmt)
.with(HasWaitDescendantC)),
hasParent(mapAnyOf(whileStmt, forStmt, doStmt)),
hasParent(compoundStmt(
hasParent(mapAnyOf(whileStmt, forStmt, doStmt))))))),
this);
}
}

View File

@@ -113,10 +113,8 @@ void SuspiciousStringCompareCheck::registerMatchers(MatchFinder *Finder) {
// Detect suspicious calls to string compare:
// 'if (strcmp())' -> 'if (strcmp() != 0)'
Finder->addMatcher(
stmt(anyOf(ifStmt(hasCondition(StringCompareCallExpr)),
whileStmt(hasCondition(StringCompareCallExpr)),
doStmt(hasCondition(StringCompareCallExpr)),
forStmt(hasCondition(StringCompareCallExpr)),
stmt(anyOf(mapAnyOf(ifStmt, whileStmt, doStmt, forStmt)
.with(hasCondition(StringCompareCallExpr)),
binaryOperator(hasAnyOperatorName("&&", "||"),
hasEitherOperand(StringCompareCallExpr))))
.bind("missing-comparison"),

View File

@@ -40,12 +40,9 @@ void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
// Self-check: Code compares something with 'this' pointer. We don't check
// whether it is actually the parameter what we compare.
const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
has(ignoringParenCasts(cxxThisExpr())))),
hasDescendant(cxxOperatorCallExpr(
hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
has(ignoringParenCasts(cxxThisExpr())))))));
const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant(
binaryOperation(hasAnyOperatorName("==", "!="),
hasEitherOperand(ignoringParenCasts(cxxThisExpr()))))));
// Both copy-and-swap and copy-and-move method creates a copy first and
// assign it to 'this' with swap or move.

View File

@@ -310,9 +310,7 @@ void UseAfterMoveFinder::getReinits(
// Assignment. In addition to the overloaded assignment operator,
// test for built-in assignment as well, since template functions
// may be instantiated to use std::move() on built-in types.
binaryOperator(hasOperatorName("="), hasLHS(DeclRefMatcher)),
cxxOperatorCallExpr(hasOverloadedOperatorName("="),
hasArgument(0, DeclRefMatcher)),
binaryOperation(hasOperatorName("="), hasLHS(DeclRefMatcher)),
// Declaration. We treat this as a type of reinitialization too,
// so we don't need to treat it separately.
declStmt(hasDescendant(equalsNode(MovedVariable))),

View File

@@ -30,12 +30,8 @@ void MutatingCopyCheck::registerMatchers(MatchFinder *Finder) {
MemberExprOrSourceObject);
const auto IsSourceMutatingAssignment = traverse(
TK_AsIs,
expr(anyOf(binaryOperator(isAssignmentOperator(), hasLHS(IsPartOfSource))
.bind(MutatingOperatorName),
cxxOperatorCallExpr(isAssignmentOperator(),
hasArgument(0, IsPartOfSource))
.bind(MutatingOperatorName))));
TK_AsIs, binaryOperation(hasOperatorName("="), hasLHS(IsPartOfSource))
.bind(MutatingOperatorName));
const auto MemberExprOrSelf = anyOf(memberExpr(), cxxThisExpr());
@@ -43,9 +39,7 @@ void MutatingCopyCheck::registerMatchers(MatchFinder *Finder) {
unless(hasDescendant(expr(unless(MemberExprOrSelf)))), MemberExprOrSelf);
const auto IsSelfMutatingAssignment =
expr(anyOf(binaryOperator(isAssignmentOperator(), hasLHS(IsPartOfSelf)),
cxxOperatorCallExpr(isAssignmentOperator(),
hasArgument(0, IsPartOfSelf))));
binaryOperation(isAssignmentOperator(), hasLHS(IsPartOfSelf));
const auto IsSelfMutatingMemberFunction =
functionDecl(hasBody(hasDescendant(IsSelfMutatingAssignment)));

View File

@@ -29,10 +29,8 @@ void AvoidGotoCheck::registerMatchers(MatchFinder *Finder) {
// Check if the 'goto' is used for control flow other than jumping
// out of a nested loop.
auto Loop = stmt(anyOf(forStmt(), cxxForRangeStmt(), whileStmt(), doStmt()));
auto NestedLoop =
stmt(anyOf(forStmt(hasAncestor(Loop)), cxxForRangeStmt(hasAncestor(Loop)),
whileStmt(hasAncestor(Loop)), doStmt(hasAncestor(Loop))));
auto Loop = mapAnyOf(forStmt, cxxForRangeStmt, whileStmt, doStmt);
auto NestedLoop = Loop.with(hasAncestor(Loop));
Finder->addMatcher(gotoStmt(anyOf(unless(hasAncestor(NestedLoop)),
unless(isForwardJumping())))

View File

@@ -47,8 +47,9 @@ void PreferIsaOrDynCastInConditionalsCheck::registerMatchers(
allOf(callee(namedDecl(hasAnyName("isa", "cast", "cast_or_null",
"dyn_cast", "dyn_cast_or_null"))
.bind("func")),
hasArgument(0, anyOf(declRefExpr().bind("arg"),
cxxMemberCallExpr().bind("arg"))))))
hasArgument(
0,
mapAnyOf(declRefExpr, cxxMemberCallExpr).bind("arg")))))
.bind("rhs");
Finder->addMatcher(

View File

@@ -188,11 +188,6 @@ StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
StatementMatcher IteratorComparisonMatcher = expr(
ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ConditionVarName)))));
auto OverloadedNEQMatcher = ignoringImplicit(
cxxOperatorCallExpr(hasOverloadedOperatorName("!="), argumentCountIs(2),
hasArgument(0, IteratorComparisonMatcher),
hasArgument(1, IteratorBoundMatcher)));
// 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.
@@ -216,14 +211,9 @@ StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
containsDeclaration(0, InitDeclMatcher),
containsDeclaration(1, EndDeclMatcher)),
declStmt(hasSingleDecl(InitDeclMatcher)))),
hasCondition(
anyOf(binaryOperator(hasOperatorName("!="),
hasLHS(IteratorComparisonMatcher),
hasRHS(IteratorBoundMatcher)),
binaryOperator(hasOperatorName("!="),
hasLHS(IteratorBoundMatcher),
hasRHS(IteratorComparisonMatcher)),
OverloadedNEQMatcher)),
hasCondition(ignoringImplicit(binaryOperation(
hasOperatorName("!="), hasOperands(IteratorComparisonMatcher,
IteratorBoundMatcher)))),
hasIncrement(anyOf(
unaryOperator(hasOperatorName("++"),
hasUnaryOperand(declRefExpr(

View File

@@ -183,15 +183,10 @@ static bool isCopyAssignmentAndCanBeDefaulted(ASTContext *Context,
auto LHS = memberExpr(hasObjectExpression(cxxThisExpr()),
member(fieldDecl(equalsNode(Field))));
auto RHS = accessToFieldInVar(Field, Param);
if (match(
traverse(TK_AsIs,
compoundStmt(has(ignoringParenImpCasts(stmt(anyOf(
binaryOperator(hasOperatorName("="), hasLHS(LHS),
hasRHS(RHS)),
cxxOperatorCallExpr(
hasOverloadedOperatorName("="), argumentCountIs(2),
hasArgument(0, LHS), hasArgument(1, RHS)))))))),
*Compound, *Context)
if (match(traverse(TK_AsIs,
compoundStmt(has(ignoringParenImpCasts(binaryOperation(
hasOperatorName("="), hasLHS(LHS), hasRHS(RHS)))))),
*Compound, *Context)
.empty())
return false;
}

View File

@@ -47,13 +47,11 @@ void MoveConstArgCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(MoveCallMatcher, this);
auto ConstParamMatcher = forEachArgumentWithParam(
MoveCallMatcher, parmVarDecl(hasType(references(isConstQualified()))));
Finder->addMatcher(callExpr(ConstParamMatcher).bind("receiving-expr"), this);
Finder->addMatcher(
traverse(TK_AsIs,
cxxConstructExpr(ConstParamMatcher).bind("receiving-expr")),
invocation(forEachArgumentWithParam(
MoveCallMatcher,
parmVarDecl(hasType(references(isConstQualified())))))
.bind("receiving-expr"),
this);
}

View File

@@ -56,26 +56,23 @@ AST_MATCHER(Expr, usedInBooleanContext) {
const char *ExprName = "__booleanContextExpr";
auto Result =
expr(expr().bind(ExprName),
anyOf(hasParent(varDecl(hasType(booleanType()))),
anyOf(hasParent(
mapAnyOf(varDecl, fieldDecl).with(hasType(booleanType()))),
hasParent(cxxConstructorDecl(
hasAnyConstructorInitializer(cxxCtorInitializer(
withInitializer(expr(equalsBoundNode(ExprName))),
forField(hasType(booleanType())))))),
hasParent(fieldDecl(hasType(booleanType()))),
hasParent(stmt(anyOf(
explicitCastExpr(hasDestinationType(booleanType())),
ifStmt(hasCondition(expr(equalsBoundNode(ExprName)))),
doStmt(hasCondition(expr(equalsBoundNode(ExprName)))),
whileStmt(hasCondition(expr(equalsBoundNode(ExprName)))),
forStmt(hasCondition(expr(equalsBoundNode(ExprName)))),
conditionalOperator(
hasCondition(expr(equalsBoundNode(ExprName)))),
mapAnyOf(ifStmt, doStmt, whileStmt, forStmt,
conditionalOperator)
.with(hasCondition(expr(equalsBoundNode(ExprName)))),
parenListExpr(hasParent(varDecl(hasType(booleanType())))),
parenExpr(hasParent(
explicitCastExpr(hasDestinationType(booleanType())))),
returnStmt(forFunction(returns(booleanType()))),
cxxUnresolvedConstructExpr(hasType(booleanType())),
callExpr(hasAnyArgumentWithParam(
invocation(hasAnyArgumentWithParam(
expr(equalsBoundNode(ExprName)),
parmVarDecl(hasType(booleanType())))),
binaryOperator(hasAnyOperatorName("&&", "||")),
@@ -181,21 +178,12 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
expr(hasType(pointsTo(ValidContainer))).bind("Pointee"))),
expr(hasType(ValidContainer)).bind("STLObject"));
Finder->addMatcher(
cxxOperatorCallExpr(
unless(isInTemplateInstantiation()),
hasAnyOverloadedOperatorName("==", "!="),
anyOf(allOf(hasArgument(0, WrongComparend), hasArgument(1, STLArg)),
allOf(hasArgument(0, STLArg), hasArgument(1, WrongComparend))),
unless(hasAncestor(
cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
.bind("BinCmp"),
this);
Finder->addMatcher(
binaryOperator(hasAnyOperatorName("==", "!="),
anyOf(allOf(hasLHS(WrongComparend), hasRHS(STLArg)),
allOf(hasLHS(STLArg), hasRHS(WrongComparend))),
unless(hasAncestor(
cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
binaryOperation(unless(isInTemplateInstantiation()),
hasAnyOperatorName("==", "!="),
hasOperands(ignoringParenImpCasts(WrongComparend),
ignoringParenImpCasts(STLArg)),
unless(hasAncestor(cxxMethodDecl(
ofClass(equalsBoundNode("container"))))))
.bind("BinCmp"),
this);
}
@@ -206,6 +194,8 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
Result.Nodes.getNodeAs<Expr>("MemberCallObject");
const auto *BinCmp = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("BinCmp");
const auto *BinCmpTempl = Result.Nodes.getNodeAs<BinaryOperator>("BinCmp");
const auto *BinCmpRewritten =
Result.Nodes.getNodeAs<CXXRewrittenBinaryOperator>("BinCmp");
const auto *BinaryOp = Result.Nodes.getNodeAs<BinaryOperator>("SizeBinaryOp");
const auto *Pointee = Result.Nodes.getNodeAs<Expr>("Pointee");
const auto *E =
@@ -236,6 +226,12 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
}
Hint = FixItHint::CreateReplacement(BinCmpTempl->getSourceRange(),
ReplacementText);
} else if (BinCmpRewritten) {
if (BinCmpRewritten->getOpcode() == BinaryOperatorKind::BO_NE) {
ReplacementText = "!" + ReplacementText;
}
Hint = FixItHint::CreateReplacement(BinCmpRewritten->getSourceRange(),
ReplacementText);
} else if (BinaryOp) { // Determine the correct transformation.
bool Negation = false;
const bool ContainerIsLHS =
@@ -313,8 +309,11 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
"for emptiness instead of 'size'")
<< Hint;
} else {
WarnLoc = BinCmpTempl ? BinCmpTempl->getBeginLoc()
: (BinCmp ? BinCmp->getBeginLoc() : SourceLocation{});
WarnLoc = BinCmpTempl
? BinCmpTempl->getBeginLoc()
: (BinCmp ? BinCmp->getBeginLoc()
: (BinCmpRewritten ? BinCmpRewritten->getBeginLoc()
: SourceLocation{}));
diag(WarnLoc, "the 'empty' method should be used to check "
"for emptiness instead of comparing to an empty object")
<< Hint;

View File

@@ -37,11 +37,10 @@ void RedundantControlFlowCheck::registerMatchers(MatchFinder *Finder) {
has(compoundStmt(hasAnySubstatement(returnStmt(unless(has(expr())))))
.bind("return"))),
this);
auto CompoundContinue =
has(compoundStmt(hasAnySubstatement(continueStmt())).bind("continue"));
Finder->addMatcher(
stmt(anyOf(forStmt(), cxxForRangeStmt(), whileStmt(), doStmt()),
CompoundContinue),
mapAnyOf(forStmt, cxxForRangeStmt, whileStmt, doStmt)
.with(hasBody(compoundStmt(hasAnySubstatement(continueStmt()))
.bind("continue"))),
this);
}

View File

@@ -66,10 +66,7 @@ constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue))));
auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
extractNodesByIdTo(Matches, "declRef", DeclRefs);
Matches =
match(findAll(cxxConstructExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
Matches = match(findAll(invocation(UsedAsConstRefOrValueArg)), Stmt, Context);
extractNodesByIdTo(Matches, "declRef", DeclRefs);
// References and pointers to const assignments.
Matches =

View File

@@ -0,0 +1,60 @@
// RUN: %check_clang_tidy -std=c++20 %s modernize-loop-convert %t -- -- -I %S/Inputs/modernize-loop-convert
namespace std {
struct strong_ordering {
int n;
constexpr operator int() const { return n; }
static const strong_ordering equal, greater, less;
};
constexpr strong_ordering strong_ordering::equal = {0};
constexpr strong_ordering strong_ordering::greater = {1};
constexpr strong_ordering strong_ordering::less = {-1};
} // namespace std
struct HasSpaceshipMem {
typedef int value_type;
struct iterator {
value_type &operator*();
const value_type &operator*() const;
iterator &operator++();
void insert(value_type);
value_type X;
constexpr auto operator<=>(const HasSpaceshipMem::iterator &) const = default;
};
iterator begin();
iterator end();
};
struct OpEqOnly {
typedef int value_type;
struct iterator {
value_type &operator*();
const value_type &operator*() const;
iterator &operator++();
bool operator==(const iterator &other) const;
void insert(value_type);
value_type X;
};
iterator begin();
iterator end();
};
void rewritten() {
OpEqOnly Oeo;
for (OpEqOnly::iterator It = Oeo.begin(), E = Oeo.end(); It != E; ++It) {
(void)*It;
}
// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
// CHECK-FIXES: for (int & It : Oeo)
// CHECK-FIXES-NEXT: (void)It;
HasSpaceshipMem Hsm;
for (HasSpaceshipMem::iterator It = Hsm.begin(), E = Hsm.end(); It != E; ++It) {
(void)*It;
}
// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
// CHECK-FIXES: for (int & It : Hsm)
// CHECK-FIXES-NEXT: (void)It;
}

View File

@@ -0,0 +1,44 @@
// RUN: %check_clang_tidy -std=c++20 %s readability-container-size-empty %t -- -- -fno-delayed-template-parsing
namespace std {
struct strong_ordering {
int n;
constexpr operator int() const { return n; }
static const strong_ordering equal, greater, less;
};
constexpr strong_ordering strong_ordering::equal = {0};
constexpr strong_ordering strong_ordering::greater = {1};
constexpr strong_ordering strong_ordering::less = {-1};
} // namespace std
template <typename T>
struct OpEqOnly {
OpEqOnly();
bool operator==(const OpEqOnly<T> &other) const;
unsigned long size() const;
bool empty() const;
};
template <typename T>
struct HasSpaceshipMem {
HasSpaceshipMem();
bool operator<=>(const HasSpaceshipMem<T> &other) const = default;
unsigned long size() const;
bool empty() const;
};
void returnsVoid() {
OpEqOnly<int> OEO;
HasSpaceshipMem<int> HSM;
if (OEO != OpEqOnly<int>())
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness
// CHECK-FIXES: {{^ }}if (!OEO.empty()){{$}}
// CHECK-MESSAGES: :19:8: note: method 'OpEqOnly'::empty() defined here
if (HSM != HasSpaceshipMem<int>())
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness
// CHECK-FIXES: {{^ }}if (!HSM.empty()){{$}}
// CHECK-MESSAGES: :27:8: note: method 'HasSpaceshipMem'::empty() defined here
}