diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h --- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h +++ b/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 allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context); diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp --- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp +++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp @@ -37,26 +37,46 @@ Nodes.insert(Match.getNodeAs(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 -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 +void constReferencedExprs( + const Matcher &M, const Stmt &Stmt, ASTContext &Context, + SmallPtrSet &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 DeclRefs; - extractNodesByIdTo(Matches, "declRef", DeclRefs); + for (const auto &Match : Matches) { + const auto *E = Match.template getNodeAs("referencedExpr"); + const auto *Call = Match.template getNodeAs("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("var"); + // Check that each non-const reference variable is also used as const. + if (isOnlyUsedAsConst(*Var, Stmt, Context)) { + const auto *E = Match.template getNodeAs("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 +constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, + ASTContext &Context) { + auto DeclRefToVar = declRefExpr(to(varDecl(equalsNode(&VarDecl)))); + SmallPtrSet 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 ConstReferencedExprs; + constReferencedExprs(ExprMatcher, Stmt, Context, ConstReferencedExprs); + return ConstReferencedExprs.size() == 1; } SmallPtrSet @@ -102,9 +164,9 @@ auto Matches = match( findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")), Stmt, Context); - SmallPtrSet DeclRefs; - extractNodesByIdTo(Matches, "declRef", DeclRefs); - return DeclRefs; + SmallPtrSet ConstReferencesToExpr; + extractNodesByIdTo(Matches, "declRef", ConstReferencesToExpr); + return ConstReferencesToExpr; } SmallPtrSet @@ -113,9 +175,9 @@ decl(forEachDescendant( declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef"))), Decl, Context); - SmallPtrSet DeclRefs; - extractNodesByIdTo(Matches, "declRef", DeclRefs); - return DeclRefs; + SmallPtrSet ConstReferencesToExpr; + extractNodesByIdTo(Matches, "declRef", ConstReferencesToExpr); + return ConstReferencesToExpr; } bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl, diff --git a/clang-tools-extra/clang-tidy/utils/Matchers.h b/clang-tools-extra/clang-tidy/utils/Matchers.h --- a/clang-tools-extra/clang-tidy/utils/Matchers.h +++ b/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. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp +++ b/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(); +}