flx created this revision.
flx added reviewers: aaron.ballman, hokein, ymandel.
Herald added a subscriber: xazax.hun.
flx requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
This change extends isOnlyUsedAsConst() to avoid false positives in cases where
a const method returns a mutable pointer or refernce and the pointed to object
is modified.
To achieve this we look at each const method or operator call and check if it
returns a mutable pointer/reference. If that's the case the call expression
itself is checked for constant use.
We also capture assignments of expressions to non-const references/pointers and
then check that the declared alias variables are used in a const-only fasion as
well.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D103087
Files:
clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
clang-tools-extra/clang-tidy/utils/Matchers.h
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -6,6 +6,9 @@
const ExpensiveToCopyType &reference() const;
void nonConstMethod();
bool constMethod() const;
+ ExpensiveToCopyType &operator*() const;
+ ExpensiveToCopyType *operator->() const;
+ ExpensiveToCopyType *get() const;
};
struct TrivialToCopyType {
@@ -508,3 +511,79 @@
// CHECK-FIXES: const auto& UnnecessaryCopy = Ref.reference();
Orig.constMethod();
}
+
+void negativePointerIsModifiedThroughConstOperator() {
+ ExpensiveToCopyType Orig;
+ auto NecessaryCopy = Orig;
+ NecessaryCopy->nonConstMethod();
+}
+
+void negativeReferenceIsModifiedThroughConstOperator() {
+ ExpensiveToCopyType Orig;
+ auto NecessaryCopy = Orig;
+ (*NecessaryCopy).nonConstMethod();
+}
+
+void negativePointerIsModifiedThroughConstOperatorChain() {
+ ExpensiveToCopyType Orig;
+ auto NecessaryCopy = Orig;
+ (*NecessaryCopy)->nonConstMethod();
+}
+
+void positivePointerIsNotModifiedThroughConstOperator() {
+ ExpensiveToCopyType Orig;
+ auto UnnecessaryCopy = Orig;
+ // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+ // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+ UnnecessaryCopy->constMethod();
+}
+
+void positiveReferenceIsNotModifiedThroughConstOperator() {
+ ExpensiveToCopyType Orig;
+ auto UnnecessaryCopy = Orig;
+ // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+ // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+ (*UnnecessaryCopy).constMethod();
+}
+
+void negativeReferenceIsAssignedToVarAndModified() {
+ ExpensiveToCopyType Orig;
+ auto NecessaryCopy = Orig;
+ auto &Ref = *NecessaryCopy;
+ Ref.nonConstMethod();
+}
+
+void positiveReferenceIsAssignedToVarAndNotModified() {
+ ExpensiveToCopyType Orig;
+ auto UnnecessaryCopy = Orig;
+ // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+ // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+ auto &Ref = *UnnecessaryCopy;
+ Ref.constMethod();
+}
+
+void negativePointerIsAssignedToVarAndModified() {
+ ExpensiveToCopyType Orig;
+ auto NecessaryCopy = Orig;
+ auto *Pointer = NecessaryCopy.get();
+ Pointer->nonConstMethod();
+}
+
+void positivePointerIsAssignedToVarAndNotModified() {
+ ExpensiveToCopyType Orig;
+ auto UnnecessaryCopy = Orig;
+ // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+ // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+ auto *Pointer = UnnecessaryCopy.get();
+ Pointer->constMethod();
+}
+
+void positiveConstMethodReturningPointer() {
+ ExpensiveToCopyType Orig;
+ auto UnnecessaryCopy = Orig;
+ // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+ // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+ // Test that a const method that returns a mutable pointer/reference that is
+ // unused counts as a const use.
+ UnnecessaryCopy.get();
+}
Index: clang-tools-extra/clang-tidy/utils/Matchers.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/Matchers.h
+++ clang-tools-extra/clang-tidy/utils/Matchers.h
@@ -43,12 +43,24 @@
return referenceType(pointee(qualType(isConstQualified())));
}
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToNonConst) {
+ using namespace ast_matchers;
+ return allOf(referenceType(),
+ referenceType(pointee(qualType(unless(isConstQualified())))));
+}
+
// Returns QualType matcher for pointers to const.
AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isPointerToConst) {
using namespace ast_matchers;
return pointerType(pointee(qualType(isConstQualified())));
}
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isPointerToNonConst) {
+ using namespace ast_matchers;
+ return allOf(pointerType(),
+ pointerType(pointee(qualType(unless(isConstQualified())))));
+}
+
// A matcher implementation that matches a list of type name regular expressions
// against a NamedDecl. If a regular expression contains the substring "::"
// matching will occur against the qualified name, otherwise only the typename.
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
@@ -27,6 +27,9 @@
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
ASTContext &Context);
+// Returns true if expression is only used in a const compatible fashion.
+bool isOnlyUsedAsConst(const Expr &E, const Stmt &Stmt, ASTContext &Context);
+
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``.
llvm::SmallPtrSet<const DeclRefExpr *, 16>
allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context);
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -37,26 +37,46 @@
Nodes.insert(Match.getNodeAs<Node>(ID));
}
-} // namespace
+bool hasMutableReturnType(const CallExpr &CallExpr, ASTContext &Context) {
+ auto Matches =
+ match(callExpr(callee(
+ functionDecl(returns(anyOf(matchers::isPointerToNonConst(),
+ matchers::isReferenceToNonConst())))
+ .bind("functionDecl"))),
+ CallExpr, Context);
+ return !Matches.empty();
+}
-// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
-// is the a const reference or value argument to a CallExpr or CXXConstructExpr.
-SmallPtrSet<const DeclRefExpr *, 16>
-constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
- ASTContext &Context) {
- auto DeclRefToVar =
- declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
+// Finds all occurrences of an expression matcher `M` where the expression is
+// used in a const fashion.
+template <typename Matcher, typename Type>
+void constReferencedExprs(
+ const Matcher &M, const Stmt &Stmt, ASTContext &Context,
+ SmallPtrSet<const Type *, 16> &ConstReferencesToExpr) {
+ auto BoundExpr = M.bind("referencedExpr");
auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
- // Match method call expressions where the variable is referenced as the this
- // implicit object argument and opertor call expression for member operators
- // where the variable is the 0-th argument.
+ // Match method call expressions where the expression is referenced as the
+ // `this` implicit object argument and operator call expressions for member
+ // operators where the variable is the 0-th argument.
auto Matches = match(
- findAll(expr(anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
- cxxOperatorCallExpr(ConstMethodCallee,
- hasArgument(0, DeclRefToVar))))),
+ findAll(expr(anyOf(
+ cxxMemberCallExpr(ConstMethodCallee, on(BoundExpr)).bind("callExpr"),
+ cxxOperatorCallExpr(ConstMethodCallee, hasArgument(0, BoundExpr))
+ .bind("callExpr")))),
Stmt, Context);
- SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ for (const auto &Match : Matches) {
+ const auto *E = Match.template getNodeAs<Type>("referencedExpr");
+ const auto *Call = Match.template getNodeAs<CallExpr>("callExpr");
+ // Examine whether the call expression returns a mutable return type, if so
+ // make sure that the call expression is used as const as well.
+ if (!hasMutableReturnType(*Call, Context) ||
+ isOnlyUsedAsConst(*Call, Stmt, Context)) {
+ ConstReferencesToExpr.insert(E);
+ }
+ }
+
+ // The expression is used as an argument to a function that takes a value or
+ // const reference parameter.
auto ConstReferenceOrValue =
qualType(anyOf(matchers::isReferenceToConst(),
unless(anyOf(referenceType(), pointerType(),
@@ -65,24 +85,56 @@
ConstReferenceOrValue,
substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue))));
auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
- DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
+ BoundExpr, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
Matches = match(findAll(invocation(UsedAsConstRefOrValueArg)), Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ extractNodesByIdTo(Matches, "referencedExpr", ConstReferencesToExpr);
+
// References and pointers to const assignments.
- Matches =
- match(findAll(declStmt(
- has(varDecl(hasType(qualType(matchers::isReferenceToConst())),
- hasInitializer(ignoringImpCasts(DeclRefToVar)))))),
- Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ Matches = match(findAll(declStmt(has(
+ varDecl(hasType(qualType(matchers::isReferenceToConst())),
+ hasInitializer(ignoringImpCasts(BoundExpr)))))),
+ Stmt, Context);
+ extractNodesByIdTo(Matches, "referencedExpr", ConstReferencesToExpr);
Matches =
match(findAll(declStmt(has(varDecl(
hasType(qualType(matchers::isPointerToConst())),
hasInitializer(ignoringImpCasts(unaryOperator(
- hasOperatorName("&"), hasUnaryOperand(DeclRefToVar)))))))),
+ hasOperatorName("&"), hasUnaryOperand(BoundExpr)))))))),
Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
- return DeclRefs;
+ extractNodesByIdTo(Matches, "referencedExpr", ConstReferencesToExpr);
+
+ // Non-const references and pointers assignments.
+ Matches = match(
+ findAll(declStmt(has(varDecl(hasType(hasCanonicalType(qualType(anyOf(
+ matchers::isReferenceToNonConst(),
+ matchers::isPointerToNonConst())))),
+ hasInitializer(ignoringImpCasts(BoundExpr)))
+ .bind("var")))),
+ Stmt, Context);
+ for (const auto &Match : Matches) {
+ const auto *Var = Match.template getNodeAs<VarDecl>("var");
+ // Check that each non-const reference variable is also used as const.
+ if (isOnlyUsedAsConst(*Var, Stmt, Context)) {
+ const auto *E = Match.template getNodeAs<Type>("referencedExpr");
+ ConstReferencesToExpr.insert(E);
+ }
+ }
+ // Parent is a compound statement. This catches cases where a CallExpr that
+ // returns a mutable pointer or reference is matched, but is not used.
+ Matches =
+ match(findAll(expr(BoundExpr, hasParent(compoundStmt()))), Stmt, Context);
+ extractNodesByIdTo(Matches, "referencedExpr", ConstReferencesToExpr);
+}
+
+} // namespace
+
+SmallPtrSet<const DeclRefExpr *, 16>
+constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
+ ASTContext &Context) {
+ auto DeclRefToVar = declRefExpr(to(varDecl(equalsNode(&VarDecl))));
+ SmallPtrSet<const DeclRefExpr *, 16> ConstDeclRefExprs;
+ constReferencedExprs(DeclRefToVar, Stmt, Context, ConstDeclRefExprs);
+ return ConstDeclRefExprs;
}
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
@@ -92,9 +144,19 @@
// reference parameter.
// If the difference is empty it is safe for the loop variable to be a const
// reference.
- auto AllDeclRefs = allDeclRefExprs(Var, Stmt, Context);
- auto ConstReferenceDeclRefs = constReferenceDeclRefExprs(Var, Stmt, Context);
- return isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs);
+ auto AllConstReferencesToExpr = allDeclRefExprs(Var, Stmt, Context);
+ auto ConstReferenceConstReferencesToExpr =
+ constReferenceDeclRefExprs(Var, Stmt, Context);
+ return isSetDifferenceEmpty(AllConstReferencesToExpr,
+ ConstReferenceConstReferencesToExpr);
+}
+
+bool isOnlyUsedAsConst(const Expr &E, const Stmt &Stmt, ASTContext &Context) {
+ auto ExprMatcher = expr(equalsNode(&E));
+ auto Matches = match(findAll(ExprMatcher), Stmt, Context);
+ SmallPtrSet<const Expr *, 16> ConstReferencedExprs;
+ constReferencedExprs(ExprMatcher, Stmt, Context, ConstReferencedExprs);
+ return ConstReferencedExprs.size() == 1;
}
SmallPtrSet<const DeclRefExpr *, 16>
@@ -102,9 +164,9 @@
auto Matches = match(
findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")),
Stmt, Context);
- SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
- return DeclRefs;
+ SmallPtrSet<const DeclRefExpr *, 16> ConstReferencesToExpr;
+ extractNodesByIdTo(Matches, "declRef", ConstReferencesToExpr);
+ return ConstReferencesToExpr;
}
SmallPtrSet<const DeclRefExpr *, 16>
@@ -113,9 +175,9 @@
decl(forEachDescendant(
declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef"))),
Decl, Context);
- SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
- return DeclRefs;
+ SmallPtrSet<const DeclRefExpr *, 16> ConstReferencesToExpr;
+ extractNodesByIdTo(Matches, "declRef", ConstReferencesToExpr);
+ return ConstReferencesToExpr;
}
bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
