diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp @@ -19,6 +19,14 @@ namespace performance { namespace { +using namespace ::clang::ast_matchers; +using llvm::StringRef; +using utils::decl_ref_expr::isOnlyUsedAsConst; + +static constexpr StringRef ObjectArgId = "objectArg"; +static constexpr StringRef InitFunctionCallId = "initFunctionCall"; +static constexpr StringRef OldVarDeclId = "oldVarDecl"; + void recordFixes(const VarDecl &Var, ASTContext &Context, DiagnosticBuilder &Diagnostic) { Diagnostic << utils::fixit::changeVarDeclToReference(Var, Context); @@ -29,10 +37,88 @@ } } -} // namespace +AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) { + // Match method call expressions where the `this` argument is only used as + // const, this will be checked in `check()` part. This returned const + // reference is highly likely to outlive the local const reference of the + // variable being declared. The assumption is that the const reference being + // returned either points to a global static variable or to a member of the + // called object. + return cxxMemberCallExpr( + callee(cxxMethodDecl(returns(matchers::isReferenceToConst()))), + on(declRefExpr(to(varDecl().bind(ObjectArgId))))); +} -using namespace ::clang::ast_matchers; -using utils::decl_ref_expr::isOnlyUsedAsConst; +AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) { + // Only allow initialization of a const reference from a free function if it + // has no arguments. Otherwise it could return an alias to one of its + // arguments and the arguments need to be checked for const use as well. + return callExpr(callee(functionDecl(returns(matchers::isReferenceToConst()))), + argumentCountIs(0), unless(callee(cxxMethodDecl()))) + .bind(InitFunctionCallId); +} + +AST_MATCHER_FUNCTION(StatementMatcher, isInitializedFromReferenceToConst) { + auto OldVarDeclRef = + declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId))); + return declStmt(has(varDecl(hasInitializer( + anyOf(isConstRefReturningFunctionCall(), isConstRefReturningMethodCall(), + ignoringImpCasts(OldVarDeclRef), + ignoringImpCasts(unaryOperator( + hasOperatorName("&"), hasUnaryOperand(OldVarDeclRef)))))))); +} + +// This checks that the variable itself is only used as const, and also makes +// sure that it does not reference another variable that could be modified in +// the BlockStmt. It does this by checking the following: +// 1. If the variable is neither a reference nor a pointer then the +// isOnlyUsedAsConst() check is sufficient. +// 2. If the (reference or pointer) variable is not initialized in a DeclStmt in +// the BlockStmt. In this case its pointee is likely not modified (unless it +// is passed as an alias into the method as well). +// 3. If the reference is initialized from a reference to const. This is +// the same set of criteria we apply when identifying the unnecessary copied +// variable in this check to begin with. In this case we check whether the +// object arg or variable that is referenced is immutable as well. +static bool isInitializingVariableImmutable(const VarDecl &InitializingVar, + const Stmt &BlockStmt, + ASTContext &Context) { + if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context)) + return false; + + QualType T = InitializingVar.getType(); + // The variable is a value type and we know it is only used as const. Safe + // to reference it and avoid the copy. + if (!isa(T)) + return true; + + auto Matches = + match(findAll(declStmt(has(varDecl(equalsNode(&InitializingVar)))) + .bind("declStmt")), + BlockStmt, Context); + // The reference or pointer is not initialized in the BlockStmt. We assume + // its pointee is not modified then. + if (Matches.empty()) + return true; + + const auto *Initialization = selectFirst("declStmt", Matches); + Matches = + match(isInitializedFromReferenceToConst(), *Initialization, Context); + // The reference is initialized from a free function without arguments + // returning a const reference. This is a global immutable object. + if (selectFirst(InitFunctionCallId, Matches) != nullptr) + return true; + // Check that the object argument is immutable as well. + if (const auto *OrigVar = selectFirst(ObjectArgId, Matches)) + return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context); + // Check that the old variable we reference is immutable as well. + if (const auto *OrigVar = selectFirst(OldVarDeclId, Matches)) + return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context); + + return false; +} + +} // namespace UnnecessaryCopyInitialization::UnnecessaryCopyInitialization( StringRef Name, ClangTidyContext *Context) @@ -41,22 +127,6 @@ utils::options::parseStringList(Options.get("AllowedTypes", ""))) {} void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) { - auto ConstReference = referenceType(pointee(qualType(isConstQualified()))); - - // Match method call expressions where the `this` argument is only used as - // const, this will be checked in `check()` part. This returned const - // reference is highly likely to outlive the local const reference of the - // variable being declared. The assumption is that the const reference being - // returned either points to a global static variable or to a member of the - // called object. - auto ConstRefReturningMethodCall = - cxxMemberCallExpr(callee(cxxMethodDecl(returns(ConstReference))), - on(declRefExpr(to(varDecl().bind("objectArg"))))); - auto ConstRefReturningFunctionCall = - callExpr(callee(functionDecl(returns(ConstReference))), - unless(callee(cxxMethodDecl()))) - .bind("initFunctionCall"); - auto localVarCopiedFrom = [this](const internal::Matcher &CopyCtorArg) { return compoundStmt( forEachDescendant( @@ -83,24 +153,22 @@ .bind("blockStmt"); }; - Finder->addMatcher(localVarCopiedFrom(anyOf(ConstRefReturningFunctionCall, - ConstRefReturningMethodCall)), + Finder->addMatcher(localVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(), + isConstRefReturningMethodCall())), this); Finder->addMatcher(localVarCopiedFrom(declRefExpr( - to(varDecl(hasLocalStorage()).bind("oldVarDecl")))), + to(varDecl(hasLocalStorage()).bind(OldVarDeclId)))), this); } void UnnecessaryCopyInitialization::check( const MatchFinder::MatchResult &Result) { const auto *NewVar = Result.Nodes.getNodeAs("newVarDecl"); - const auto *OldVar = Result.Nodes.getNodeAs("oldVarDecl"); - const auto *ObjectArg = Result.Nodes.getNodeAs("objectArg"); + const auto *OldVar = Result.Nodes.getNodeAs(OldVarDeclId); + const auto *ObjectArg = Result.Nodes.getNodeAs(ObjectArgId); const auto *BlockStmt = Result.Nodes.getNodeAs("blockStmt"); const auto *CtorCall = Result.Nodes.getNodeAs("ctorCall"); - const auto *InitFunctionCall = - Result.Nodes.getNodeAs("initFunctionCall"); TraversalKindScope RAII(*Result.Context, ast_type_traits::TK_AsIs); @@ -118,11 +186,6 @@ return; if (OldVar == nullptr) { - // Only allow initialization of a const reference from a free function if it - // has no arguments. Otherwise it could return an alias to one of its - // arguments and the arguments need to be checked for const use as well. - if (InitFunctionCall != nullptr && InitFunctionCall->getNumArgs() > 0) - return; handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg, *Result.Context); } else { @@ -138,7 +201,7 @@ if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context)) return; if (ObjectArg != nullptr && - !isOnlyUsedAsConst(*ObjectArg, BlockStmt, Context)) + !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context)) return; auto Diagnostic = @@ -158,7 +221,7 @@ const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt, bool IssueFix, ASTContext &Context) { if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) || - !isOnlyUsedAsConst(OldVar, BlockStmt, Context)) + !isInitializingVariableImmutable(OldVar, BlockStmt, Context)) return; auto Diagnostic = diag(NewVar.getLocation(), 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 @@ -58,7 +58,7 @@ SmallPtrSet DeclRefs; extractNodesByIdTo(Matches, "declRef", DeclRefs); auto ConstReferenceOrValue = - qualType(anyOf(referenceType(pointee(qualType(isConstQualified()))), + qualType(anyOf(matchers::isReferenceToConst(), unless(anyOf(referenceType(), pointerType(), substTemplateTypeParmType())))); auto ConstReferenceOrValueOrReplaced = qualType(anyOf( @@ -71,6 +71,20 @@ Matches = match(findAll(cxxConstructExpr(UsedAsConstRefOrValueArg)), Stmt, Context); extractNodesByIdTo(Matches, "declRef", DeclRefs); + // 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::isPointerToConst())), + hasInitializer(ignoringImpCasts(unaryOperator( + hasOperatorName("&"), hasUnaryOperand(DeclRefToVar)))))))), + Stmt, Context); + extractNodesByIdTo(Matches, "declRef", DeclRefs); return DeclRefs; } 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,6 +43,12 @@ return referenceType(pointee(qualType(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_P(NamedDecl, matchesAnyListedName, std::vector, NameList) { return llvm::any_of(NameList, [&Node](const std::string &Name) { 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 @@ -476,3 +476,35 @@ auto Copy = Orig.reference(); Update(Copy); } + +void negativeCopiedFromReferenceToModifiedVar() { + ExpensiveToCopyType Orig; + const auto &Ref = Orig; + const auto NecessaryCopy = Ref; + Orig.nonConstMethod(); +} + +void positiveCopiedFromReferenceToConstVar() { + ExpensiveToCopyType Orig; + const auto &Ref = Orig; + const auto UnnecessaryCopy = Ref; + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: local copy 'UnnecessaryCopy' of + // CHECK-FIXES: const auto& UnnecessaryCopy = Ref; + Orig.constMethod(); +} + +void negativeCopiedFromGetterOfReferenceToModifiedVar() { + ExpensiveToCopyType Orig; + const auto &Ref = Orig.reference(); + const auto NecessaryCopy = Ref.reference(); + Orig.nonConstMethod(); +} + +void positiveCopiedFromGetterOfReferenceToConstVar() { + ExpensiveToCopyType Orig; + const auto &Ref = Orig.reference(); + auto UnnecessaryCopy = Ref.reference(); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'UnnecessaryCopy' is + // CHECK-FIXES: const auto& UnnecessaryCopy = Ref.reference(); + Orig.constMethod(); +}