[clang] [clang-tools-extra] [clang-tidy] Improve performance of misc-const-correctness (PR #72705)
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 5 11:23:47 PST 2024
https://github.com/PiotrZSL updated https://github.com/llvm/llvm-project/pull/72705
>From 1f9b443a92446ae36825152535706037ef2e87b4 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Wed, 15 Nov 2023 16:52:00 +0000
Subject: [PATCH 1/2] [clang-tidy] Improve performance of
misc-const-correctness
Replaced certain AST matchers in ExprMutationAnalyzer with a more direct
utilization of AST classes. The primary bottleneck was identified in the
canResolveToExpr AST matcher. Since this matcher was employed multiple
times and used recursively, each invocation led to the constant creation
and destruction of other matchers within it. Additionally, the continual
comparison of DynTypedNode resulted in significant performance degradation.
The optimization was tested on the TargetLowering.cpp file. Originally,
the check took 156 seconds on that file, but after implementing this
enhancement, it now takes approximately 40 seconds, making it nearly four
times faster.
Despite this improvement, there are still numerous issues in this file.
To further reduce the computational cost of this class, it is advisable to
consider removing the remaining matchers and exploring alternatives such as
leveraging RecursiveASTVisitor and increasing the direct use of AST classes.
---
clang-tools-extra/docs/ReleaseNotes.rst | 3 +-
clang/lib/Analysis/ExprMutationAnalyzer.cpp | 350 +++++++++++---------
2 files changed, 188 insertions(+), 165 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 4d25e2ebe85f5f..2960ef644fde4b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -358,7 +358,8 @@ Changes in existing checks
<clang-tidy/checks/misc/const-correctness>` check to avoid false positive when
using pointer to member function. Additionally, the check no longer emits
a diagnostic when a variable that is not type-dependent is an operand of a
- type-dependent binary operator.
+ type-dependent binary operator. Improved performance of the check through
+ optimizations.
- Improved :doc:`misc-include-cleaner
<clang-tidy/checks/misc/include-cleaner>` check by adding option
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 624a643cc60e4b..760c867ef79f74 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -15,6 +15,76 @@
namespace clang {
using namespace ast_matchers;
+static bool canResolveToExprImpl(const Expr *Node, const Expr *Target) {
+
+ const auto IgnoreDerivedToBase = [](const Expr *E, auto Matcher) {
+ if (Matcher(E))
+ return true;
+ if (const auto *Cast = dyn_cast<ImplicitCastExpr>(E)) {
+ if ((Cast->getCastKind() == CK_DerivedToBase ||
+ Cast->getCastKind() == CK_UncheckedDerivedToBase) &&
+ Matcher(Cast->getSubExpr()))
+ return true;
+ }
+ return false;
+ };
+
+ const auto EvalCommaExpr = [](const Expr *E, auto Matcher) {
+ const Expr *Result = E;
+ while (const auto *BOComma =
+ dyn_cast_or_null<BinaryOperator>(Result->IgnoreParens())) {
+ if (!BOComma->isCommaOp())
+ break;
+ Result = BOComma->getRHS();
+ }
+
+ return Result != E && Matcher(Result);
+ };
+
+ // The 'ConditionalOperatorM' matches on `<anything> ? <expr> : <expr>`.
+ // This matching must be recursive because `<expr>` can be anything resolving
+ // to the `InnerMatcher`, for example another conditional operator.
+ // The edge-case `BaseClass &b = <cond> ? DerivedVar1 : DerivedVar2;`
+ // is handled, too. The implicit cast happens outside of the conditional.
+ // This is matched by `IgnoreDerivedToBase(canResolveToExpr(InnerMatcher))`
+ // below.
+ const auto ConditionalOperatorM = [Target](const Expr *E, auto Matcher) {
+ if (const auto *OP = dyn_cast<ConditionalOperator>(E)) {
+ if (const auto *TE = OP->getTrueExpr()->IgnoreParens())
+ if (canResolveToExprImpl(TE, Target))
+ return true;
+ if (const auto *FE = OP->getFalseExpr()->IgnoreParens())
+ if (canResolveToExprImpl(FE, Target))
+ return true;
+ }
+ return false;
+ };
+
+ const auto ElvisOperator = [Target](const Expr *E, auto Matcher) {
+ if (const auto *OP = dyn_cast<BinaryConditionalOperator>(E)) {
+ if (const auto *TE = OP->getTrueExpr()->IgnoreParens())
+ if (canResolveToExprImpl(TE, Target))
+ return true;
+ if (const auto *FE = OP->getFalseExpr()->IgnoreParens())
+ if (canResolveToExprImpl(FE, Target))
+ return true;
+ }
+ return false;
+ };
+
+ const auto *EP = Node->IgnoreParens();
+ return IgnoreDerivedToBase(EP,
+ [&](const Expr *E) {
+ return E == Target ||
+ ConditionalOperatorM(E, Target) ||
+ ElvisOperator(E, Target);
+ }) ||
+ EvalCommaExpr(EP, [&](const Expr *E) {
+ return IgnoreDerivedToBase(
+ E->IgnoreParens(), [&](const Expr *EE) { return EE == Target; });
+ });
+}
+
namespace {
AST_MATCHER_P(LambdaExpr, hasCaptureInit, const Expr *, E) {
@@ -27,56 +97,14 @@ AST_MATCHER_P(CXXForRangeStmt, hasRangeStmt,
return InnerMatcher.matches(*Range, Finder, Builder);
}
-AST_MATCHER_P(Expr, maybeEvalCommaExpr, ast_matchers::internal::Matcher<Expr>,
- InnerMatcher) {
- const Expr *Result = &Node;
- while (const auto *BOComma =
- dyn_cast_or_null<BinaryOperator>(Result->IgnoreParens())) {
- if (!BOComma->isCommaOp())
- break;
- Result = BOComma->getRHS();
- }
- return InnerMatcher.matches(*Result, Finder, Builder);
-}
-
-AST_MATCHER_P(Stmt, canResolveToExpr, ast_matchers::internal::Matcher<Stmt>,
- InnerMatcher) {
+AST_MATCHER_P(Stmt, canResolveToExpr, const Stmt *, Inner) {
auto *Exp = dyn_cast<Expr>(&Node);
- if (!Exp) {
- return stmt().matches(Node, Finder, Builder);
- }
-
- auto DerivedToBase = [](const ast_matchers::internal::Matcher<Expr> &Inner) {
- return implicitCastExpr(anyOf(hasCastKind(CK_DerivedToBase),
- hasCastKind(CK_UncheckedDerivedToBase)),
- hasSourceExpression(Inner));
- };
- auto IgnoreDerivedToBase =
- [&DerivedToBase](const ast_matchers::internal::Matcher<Expr> &Inner) {
- return ignoringParens(expr(anyOf(Inner, DerivedToBase(Inner))));
- };
-
- // The 'ConditionalOperator' matches on `<anything> ? <expr> : <expr>`.
- // This matching must be recursive because `<expr>` can be anything resolving
- // to the `InnerMatcher`, for example another conditional operator.
- // The edge-case `BaseClass &b = <cond> ? DerivedVar1 : DerivedVar2;`
- // is handled, too. The implicit cast happens outside of the conditional.
- // This is matched by `IgnoreDerivedToBase(canResolveToExpr(InnerMatcher))`
- // below.
- auto const ConditionalOperator = conditionalOperator(anyOf(
- hasTrueExpression(ignoringParens(canResolveToExpr(InnerMatcher))),
- hasFalseExpression(ignoringParens(canResolveToExpr(InnerMatcher)))));
- auto const ElvisOperator = binaryConditionalOperator(anyOf(
- hasTrueExpression(ignoringParens(canResolveToExpr(InnerMatcher))),
- hasFalseExpression(ignoringParens(canResolveToExpr(InnerMatcher)))));
-
- auto const ComplexMatcher = ignoringParens(
- expr(anyOf(IgnoreDerivedToBase(InnerMatcher),
- maybeEvalCommaExpr(IgnoreDerivedToBase(InnerMatcher)),
- IgnoreDerivedToBase(ConditionalOperator),
- IgnoreDerivedToBase(ElvisOperator))));
-
- return ComplexMatcher.matches(*Exp, Finder, Builder);
+ if (!Exp)
+ return true;
+ auto *Target = dyn_cast<Expr>(Inner);
+ if (!Target)
+ return false;
+ return canResolveToExprImpl(Exp, Target);
}
// Similar to 'hasAnyArgument', but does not work because 'InitListExpr' does
@@ -121,6 +149,12 @@ AST_MATCHER_P(GenericSelectionExpr, hasControllingExpr,
return InnerMatcher.matches(*Node.getControllingExpr(), Finder, Builder);
}
+template <typename T>
+ast_matchers::internal::Matcher<T>
+findFirst(const ast_matchers::internal::Matcher<T> &Matcher) {
+ return anyOf(Matcher, hasDescendant(Matcher));
+}
+
const auto nonConstReferenceType = [] {
return hasUnqualifiedDesugaredType(
referenceType(pointee(unless(isConstQualified()))));
@@ -220,8 +254,8 @@ bool ExprMutationAnalyzer::isUnevaluated(const Stmt *Exp, const Stmt &Stm,
return selectFirst<Stmt>(
NodeID<Expr>::value,
match(
- findAll(
- stmt(canResolveToExpr(equalsNode(Exp)),
+ findFirst(
+ stmt(canResolveToExpr(Exp),
anyOf(
// `Exp` is part of the underlying expression of
// decltype/typeof if it has an ancestor of
@@ -275,44 +309,41 @@ const Stmt *ExprMutationAnalyzer::findDeclPointeeMutation(
const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
// LHS of any assignment operators.
- const auto AsAssignmentLhs = binaryOperator(
- isAssignmentOperator(), hasLHS(canResolveToExpr(equalsNode(Exp))));
+ const auto AsAssignmentLhs =
+ binaryOperator(isAssignmentOperator(), hasLHS(canResolveToExpr(Exp)));
// Operand of increment/decrement operators.
const auto AsIncDecOperand =
unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")),
- hasUnaryOperand(canResolveToExpr(equalsNode(Exp))));
+ hasUnaryOperand(canResolveToExpr(Exp)));
// Invoking non-const member function.
// A member function is assumed to be non-const when it is unresolved.
const auto NonConstMethod = cxxMethodDecl(unless(isConst()));
const auto AsNonConstThis = expr(anyOf(
- cxxMemberCallExpr(on(canResolveToExpr(equalsNode(Exp))),
- unless(isConstCallee())),
+ cxxMemberCallExpr(on(canResolveToExpr(Exp)), unless(isConstCallee())),
cxxOperatorCallExpr(callee(NonConstMethod),
- hasArgument(0, canResolveToExpr(equalsNode(Exp)))),
+ hasArgument(0, canResolveToExpr(Exp))),
// In case of a templated type, calling overloaded operators is not
// resolved and modelled as `binaryOperator` on a dependent type.
// Such instances are considered a modification, because they can modify
// in different instantiations of the template.
- binaryOperator(
- hasEitherOperand(ignoringImpCasts(canResolveToExpr(equalsNode(Exp)))),
- isTypeDependent()),
+ binaryOperator(isTypeDependent(),
+ hasEitherOperand(ignoringImpCasts(canResolveToExpr(Exp)))),
// Within class templates and member functions the member expression might
// not be resolved. In that case, the `callExpr` is considered to be a
// modification.
- callExpr(
- callee(expr(anyOf(unresolvedMemberExpr(hasObjectExpression(
- canResolveToExpr(equalsNode(Exp)))),
- cxxDependentScopeMemberExpr(hasObjectExpression(
- canResolveToExpr(equalsNode(Exp)))))))),
+ callExpr(callee(expr(anyOf(
+ unresolvedMemberExpr(hasObjectExpression(canResolveToExpr(Exp))),
+ cxxDependentScopeMemberExpr(
+ hasObjectExpression(canResolveToExpr(Exp))))))),
// Match on a call to a known method, but the call itself is type
// dependent (e.g. `vector<T> v; v.push(T{});` in a templated function).
- callExpr(allOf(isTypeDependent(),
- callee(memberExpr(hasDeclaration(NonConstMethod),
- hasObjectExpression(canResolveToExpr(
- equalsNode(Exp)))))))));
+ callExpr(allOf(
+ isTypeDependent(),
+ callee(memberExpr(hasDeclaration(NonConstMethod),
+ hasObjectExpression(canResolveToExpr(Exp))))))));
// Taking address of 'Exp'.
// We're assuming 'Exp' is mutated as soon as its address is taken, though in
@@ -322,11 +353,10 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
unaryOperator(hasOperatorName("&"),
// A NoOp implicit cast is adding const.
unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))),
- hasUnaryOperand(canResolveToExpr(equalsNode(Exp))));
- const auto AsPointerFromArrayDecay =
- castExpr(hasCastKind(CK_ArrayToPointerDecay),
- unless(hasParent(arraySubscriptExpr())),
- has(canResolveToExpr(equalsNode(Exp))));
+ hasUnaryOperand(canResolveToExpr(Exp)));
+ const auto AsPointerFromArrayDecay = castExpr(
+ hasCastKind(CK_ArrayToPointerDecay),
+ unless(hasParent(arraySubscriptExpr())), has(canResolveToExpr(Exp)));
// Treat calling `operator->()` of move-only classes as taking address.
// These are typically smart pointers with unique ownership so we treat
// mutation of pointee as mutation of the smart pointer itself.
@@ -334,7 +364,7 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
hasOverloadedOperatorName("->"),
callee(
cxxMethodDecl(ofClass(isMoveOnly()), returns(nonConstPointerType()))),
- argumentCountIs(1), hasArgument(0, canResolveToExpr(equalsNode(Exp))));
+ argumentCountIs(1), hasArgument(0, canResolveToExpr(Exp)));
// Used as non-const-ref argument when calling a function.
// An argument is assumed to be non-const-ref when the function is unresolved.
@@ -342,8 +372,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
// findFunctionArgMutation which has additional smarts for handling forwarding
// references.
const auto NonConstRefParam = forEachArgumentWithParamType(
- anyOf(canResolveToExpr(equalsNode(Exp)),
- memberExpr(hasObjectExpression(canResolveToExpr(equalsNode(Exp))))),
+ anyOf(canResolveToExpr(Exp),
+ memberExpr(hasObjectExpression(canResolveToExpr(Exp)))),
nonConstReferenceType());
const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
const auto TypeDependentCallee =
@@ -354,19 +384,17 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
const auto AsNonConstRefArg = anyOf(
callExpr(NonConstRefParam, NotInstantiated),
cxxConstructExpr(NonConstRefParam, NotInstantiated),
- callExpr(TypeDependentCallee,
- hasAnyArgument(canResolveToExpr(equalsNode(Exp)))),
- cxxUnresolvedConstructExpr(
- hasAnyArgument(canResolveToExpr(equalsNode(Exp)))),
+ callExpr(TypeDependentCallee, hasAnyArgument(canResolveToExpr(Exp))),
+ cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
// Previous False Positive in the following Code:
// `template <typename T> void f() { int i = 42; new Type<T>(i); }`
// Where the constructor of `Type` takes its argument as reference.
// The AST does not resolve in a `cxxConstructExpr` because it is
// type-dependent.
- parenListExpr(hasDescendant(expr(canResolveToExpr(equalsNode(Exp))))),
+ parenListExpr(hasDescendant(expr(canResolveToExpr(Exp)))),
// If the initializer is for a reference type, there is no cast for
// the variable. Values are cast to RValue first.
- initListExpr(hasAnyInit(expr(canResolveToExpr(equalsNode(Exp))))));
+ initListExpr(hasAnyInit(expr(canResolveToExpr(Exp)))));
// Captured by a lambda by reference.
// If we're initializing a capture with 'Exp' directly then we're initializing
@@ -380,76 +408,72 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
// For returning by const-ref there will be an ImplicitCastExpr <NoOp> (for
// adding const.)
const auto AsNonConstRefReturn =
- returnStmt(hasReturnValue(canResolveToExpr(equalsNode(Exp))));
+ returnStmt(hasReturnValue(canResolveToExpr(Exp)));
// It is used as a non-const-reference for initalizing a range-for loop.
- const auto AsNonConstRefRangeInit = cxxForRangeStmt(
- hasRangeInit(declRefExpr(allOf(canResolveToExpr(equalsNode(Exp)),
- hasType(nonConstReferenceType())))));
+ const auto AsNonConstRefRangeInit = cxxForRangeStmt(hasRangeInit(declRefExpr(
+ allOf(canResolveToExpr(Exp), hasType(nonConstReferenceType())))));
const auto Matches = match(
- traverse(TK_AsIs,
- findAll(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand,
- AsNonConstThis, AsAmpersandOperand,
- AsPointerFromArrayDecay, AsOperatorArrowThis,
- AsNonConstRefArg, AsLambdaRefCaptureInit,
- AsNonConstRefReturn, AsNonConstRefRangeInit))
- .bind("stmt"))),
+ traverse(
+ TK_AsIs,
+ findFirst(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand, AsNonConstThis,
+ AsAmpersandOperand, AsPointerFromArrayDecay,
+ AsOperatorArrowThis, AsNonConstRefArg,
+ AsLambdaRefCaptureInit, AsNonConstRefReturn,
+ AsNonConstRefRangeInit))
+ .bind("stmt"))),
Stm, Context);
return selectFirst<Stmt>("stmt", Matches);
}
const Stmt *ExprMutationAnalyzer::findMemberMutation(const Expr *Exp) {
// Check whether any member of 'Exp' is mutated.
- const auto MemberExprs =
- match(findAll(expr(anyOf(memberExpr(hasObjectExpression(
- canResolveToExpr(equalsNode(Exp)))),
- cxxDependentScopeMemberExpr(hasObjectExpression(
- canResolveToExpr(equalsNode(Exp)))),
- binaryOperator(hasOperatorName(".*"),
- hasLHS(equalsNode(Exp)))))
- .bind(NodeID<Expr>::value)),
- Stm, Context);
+ const auto MemberExprs = match(
+ findAll(expr(anyOf(memberExpr(hasObjectExpression(canResolveToExpr(Exp))),
+ cxxDependentScopeMemberExpr(
+ hasObjectExpression(canResolveToExpr(Exp))),
+ binaryOperator(hasOperatorName(".*"),
+ hasLHS(equalsNode(Exp)))))
+ .bind(NodeID<Expr>::value)),
+ Stm, Context);
return findExprMutation(MemberExprs);
}
const Stmt *ExprMutationAnalyzer::findArrayElementMutation(const Expr *Exp) {
// Check whether any element of an array is mutated.
- const auto SubscriptExprs =
- match(findAll(arraySubscriptExpr(
- anyOf(hasBase(canResolveToExpr(equalsNode(Exp))),
- hasBase(implicitCastExpr(
- allOf(hasCastKind(CK_ArrayToPointerDecay),
- hasSourceExpression(canResolveToExpr(
- equalsNode(Exp))))))))
- .bind(NodeID<Expr>::value)),
- Stm, Context);
+ const auto SubscriptExprs = match(
+ findAll(arraySubscriptExpr(
+ anyOf(hasBase(canResolveToExpr(Exp)),
+ hasBase(implicitCastExpr(allOf(
+ hasCastKind(CK_ArrayToPointerDecay),
+ hasSourceExpression(canResolveToExpr(Exp)))))))
+ .bind(NodeID<Expr>::value)),
+ Stm, Context);
return findExprMutation(SubscriptExprs);
}
const Stmt *ExprMutationAnalyzer::findCastMutation(const Expr *Exp) {
// If the 'Exp' is explicitly casted to a non-const reference type the
// 'Exp' is considered to be modified.
- const auto ExplicitCast = match(
- findAll(
- stmt(castExpr(hasSourceExpression(canResolveToExpr(equalsNode(Exp))),
- explicitCastExpr(
- hasDestinationType(nonConstReferenceType()))))
- .bind("stmt")),
- Stm, Context);
+ const auto ExplicitCast =
+ match(findFirst(stmt(castExpr(hasSourceExpression(canResolveToExpr(Exp)),
+ explicitCastExpr(hasDestinationType(
+ nonConstReferenceType()))))
+ .bind("stmt")),
+ Stm, Context);
if (const auto *CastStmt = selectFirst<Stmt>("stmt", ExplicitCast))
return CastStmt;
// If 'Exp' is casted to any non-const reference type, check the castExpr.
const auto Casts = match(
- findAll(
- expr(castExpr(hasSourceExpression(canResolveToExpr(equalsNode(Exp))),
- anyOf(explicitCastExpr(
- hasDestinationType(nonConstReferenceType())),
- implicitCastExpr(hasImplicitDestinationType(
- nonConstReferenceType())))))
- .bind(NodeID<Expr>::value)),
+ findAll(expr(castExpr(hasSourceExpression(canResolveToExpr(Exp)),
+ anyOf(explicitCastExpr(hasDestinationType(
+ nonConstReferenceType())),
+ implicitCastExpr(hasImplicitDestinationType(
+ nonConstReferenceType())))))
+ .bind(NodeID<Expr>::value)),
Stm, Context);
if (const Stmt *S = findExprMutation(Casts))
@@ -458,7 +482,7 @@ const Stmt *ExprMutationAnalyzer::findCastMutation(const Expr *Exp) {
const auto Calls =
match(findAll(callExpr(callee(namedDecl(
hasAnyName("::std::move", "::std::forward"))),
- hasArgument(0, canResolveToExpr(equalsNode(Exp))))
+ hasArgument(0, canResolveToExpr(Exp)))
.bind("expr")),
Stm, Context);
return findExprMutation(Calls);
@@ -473,16 +497,16 @@ const Stmt *ExprMutationAnalyzer::findRangeLoopMutation(const Expr *Exp) {
// array is considered modified if the loop-variable is a non-const reference.
const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType(
hasUnqualifiedDesugaredType(referenceType(pointee(arrayType())))))));
- const auto RefToArrayRefToElements =
- match(findAll(stmt(cxxForRangeStmt(
- hasLoopVariable(
- varDecl(anyOf(hasType(nonConstReferenceType()),
- hasType(nonConstPointerType())))
- .bind(NodeID<Decl>::value)),
- hasRangeStmt(DeclStmtToNonRefToArray),
- hasRangeInit(canResolveToExpr(equalsNode(Exp)))))
- .bind("stmt")),
- Stm, Context);
+ const auto RefToArrayRefToElements = match(
+ findFirst(stmt(cxxForRangeStmt(
+ hasLoopVariable(
+ varDecl(anyOf(hasType(nonConstReferenceType()),
+ hasType(nonConstPointerType())))
+ .bind(NodeID<Decl>::value)),
+ hasRangeStmt(DeclStmtToNonRefToArray),
+ hasRangeInit(canResolveToExpr(Exp))))
+ .bind("stmt")),
+ Stm, Context);
if (const auto *BadRangeInitFromArray =
selectFirst<Stmt>("stmt", RefToArrayRefToElements))
@@ -505,12 +529,12 @@ const Stmt *ExprMutationAnalyzer::findRangeLoopMutation(const Expr *Exp) {
hasSingleDecl(varDecl(hasType(hasUnqualifiedDesugaredType(referenceType(
pointee(hasDeclaration(cxxRecordDecl(HasAnyNonConstIterator)))))))));
- const auto RefToContainerBadIterators =
- match(findAll(stmt(cxxForRangeStmt(allOf(
- hasRangeStmt(DeclStmtToNonConstIteratorContainer),
- hasRangeInit(canResolveToExpr(equalsNode(Exp))))))
- .bind("stmt")),
- Stm, Context);
+ const auto RefToContainerBadIterators = match(
+ findFirst(stmt(cxxForRangeStmt(allOf(
+ hasRangeStmt(DeclStmtToNonConstIteratorContainer),
+ hasRangeInit(canResolveToExpr(Exp)))))
+ .bind("stmt")),
+ Stm, Context);
if (const auto *BadIteratorsContainer =
selectFirst<Stmt>("stmt", RefToContainerBadIterators))
@@ -522,7 +546,7 @@ const Stmt *ExprMutationAnalyzer::findRangeLoopMutation(const Expr *Exp) {
match(findAll(cxxForRangeStmt(
hasLoopVariable(varDecl(hasType(nonConstReferenceType()))
.bind(NodeID<Decl>::value)),
- hasRangeInit(canResolveToExpr(equalsNode(Exp))))),
+ hasRangeInit(canResolveToExpr(Exp)))),
Stm, Context);
return findDeclMutation(LoopVars);
}
@@ -531,31 +555,29 @@ const Stmt *ExprMutationAnalyzer::findReferenceMutation(const Expr *Exp) {
// Follow non-const reference returned by `operator*()` of move-only classes.
// These are typically smart pointers with unique ownership so we treat
// mutation of pointee as mutation of the smart pointer itself.
- const auto Ref =
- match(findAll(cxxOperatorCallExpr(
- hasOverloadedOperatorName("*"),
- callee(cxxMethodDecl(ofClass(isMoveOnly()),
- returns(nonConstReferenceType()))),
- argumentCountIs(1),
- hasArgument(0, canResolveToExpr(equalsNode(Exp))))
- .bind(NodeID<Expr>::value)),
- Stm, Context);
+ const auto Ref = match(
+ findAll(cxxOperatorCallExpr(
+ hasOverloadedOperatorName("*"),
+ callee(cxxMethodDecl(ofClass(isMoveOnly()),
+ returns(nonConstReferenceType()))),
+ argumentCountIs(1), hasArgument(0, canResolveToExpr(Exp)))
+ .bind(NodeID<Expr>::value)),
+ Stm, Context);
if (const Stmt *S = findExprMutation(Ref))
return S;
// If 'Exp' is bound to a non-const reference, check all declRefExpr to that.
const auto Refs = match(
stmt(forEachDescendant(
- varDecl(
- hasType(nonConstReferenceType()),
- hasInitializer(anyOf(canResolveToExpr(equalsNode(Exp)),
- memberExpr(hasObjectExpression(
- canResolveToExpr(equalsNode(Exp)))))),
- hasParent(declStmt().bind("stmt")),
- // Don't follow the reference in range statement, we've
- // handled that separately.
- unless(hasParent(declStmt(hasParent(
- cxxForRangeStmt(hasRangeStmt(equalsBoundNode("stmt"))))))))
+ varDecl(hasType(nonConstReferenceType()),
+ hasInitializer(anyOf(
+ canResolveToExpr(Exp),
+ memberExpr(hasObjectExpression(canResolveToExpr(Exp))))),
+ hasParent(declStmt().bind("stmt")),
+ // Don't follow the reference in range statement, we've
+ // handled that separately.
+ unless(hasParent(declStmt(hasParent(cxxForRangeStmt(
+ hasRangeStmt(equalsBoundNode("stmt"))))))))
.bind(NodeID<Decl>::value))),
Stm, Context);
return findDeclMutation(Refs);
@@ -563,7 +585,7 @@ const Stmt *ExprMutationAnalyzer::findReferenceMutation(const Expr *Exp) {
const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) {
const auto NonConstRefParam = forEachArgumentWithParam(
- canResolveToExpr(equalsNode(Exp)),
+ canResolveToExpr(Exp),
parmVarDecl(hasType(nonConstReferenceType())).bind("parm"));
const auto IsInstantiated = hasDeclaration(isInstantiated());
const auto FuncDecl = hasDeclaration(functionDecl().bind("func"));
>From 5cd572ef64c3f342b0944d98280fc1662fd03515 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Fri, 5 Jan 2024 19:21:29 +0000
Subject: [PATCH 2/2] Resolve some comments
---
clang/lib/Analysis/ExprMutationAnalyzer.cpp | 33 ++++++++++++---------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 760c867ef79f74..c0de9277ff866f 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -15,7 +15,13 @@
namespace clang {
using namespace ast_matchers;
-static bool canResolveToExprImpl(const Expr *Node, const Expr *Target) {
+// Check if result of Source expression could be a Target expression.
+// Checks:
+// - Implicit Casts
+// - Binary Operators
+// - ConditionalOperator
+// - BinaryConditionalOperator
+static bool canExprResolveTo(const Expr *Source, const Expr *Target) {
const auto IgnoreDerivedToBase = [](const Expr *E, auto Matcher) {
if (Matcher(E))
@@ -48,38 +54,37 @@ static bool canResolveToExprImpl(const Expr *Node, const Expr *Target) {
// is handled, too. The implicit cast happens outside of the conditional.
// This is matched by `IgnoreDerivedToBase(canResolveToExpr(InnerMatcher))`
// below.
- const auto ConditionalOperatorM = [Target](const Expr *E, auto Matcher) {
+ const auto ConditionalOperatorM = [Target](const Expr *E) {
if (const auto *OP = dyn_cast<ConditionalOperator>(E)) {
if (const auto *TE = OP->getTrueExpr()->IgnoreParens())
- if (canResolveToExprImpl(TE, Target))
+ if (canExprResolveTo(TE, Target))
return true;
if (const auto *FE = OP->getFalseExpr()->IgnoreParens())
- if (canResolveToExprImpl(FE, Target))
+ if (canExprResolveTo(FE, Target))
return true;
}
return false;
};
- const auto ElvisOperator = [Target](const Expr *E, auto Matcher) {
+ const auto ElvisOperator = [Target](const Expr *E) {
if (const auto *OP = dyn_cast<BinaryConditionalOperator>(E)) {
if (const auto *TE = OP->getTrueExpr()->IgnoreParens())
- if (canResolveToExprImpl(TE, Target))
+ if (canExprResolveTo(TE, Target))
return true;
if (const auto *FE = OP->getFalseExpr()->IgnoreParens())
- if (canResolveToExprImpl(FE, Target))
+ if (canExprResolveTo(FE, Target))
return true;
}
return false;
};
- const auto *EP = Node->IgnoreParens();
- return IgnoreDerivedToBase(EP,
+ const Expr *SourceExprP = Source->IgnoreParens();
+ return IgnoreDerivedToBase(SourceExprP,
[&](const Expr *E) {
- return E == Target ||
- ConditionalOperatorM(E, Target) ||
- ElvisOperator(E, Target);
+ return E == Target || ConditionalOperatorM(E) ||
+ ElvisOperator(E);
}) ||
- EvalCommaExpr(EP, [&](const Expr *E) {
+ EvalCommaExpr(SourceExprP, [&](const Expr *E) {
return IgnoreDerivedToBase(
E->IgnoreParens(), [&](const Expr *EE) { return EE == Target; });
});
@@ -104,7 +109,7 @@ AST_MATCHER_P(Stmt, canResolveToExpr, const Stmt *, Inner) {
auto *Target = dyn_cast<Expr>(Inner);
if (!Target)
return false;
- return canResolveToExprImpl(Exp, Target);
+ return canExprResolveTo(Exp, Target);
}
// Similar to 'hasAnyArgument', but does not work because 'InitListExpr' does
More information about the cfe-commits
mailing list