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 kObjectArg = "objectArg"; +static constexpr StringRef kInitFunctionCall = "initFunctionCall"; +static constexpr StringRef kOldVarDecl = "oldVarDecl"; + void recordFixes(const VarDecl &Var, ASTContext &Context, DiagnosticBuilder &Diagnostic) { Diagnostic << utils::fixit::changeVarDeclToReference(Var, Context); @@ -29,10 +37,91 @@ } } -} // 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(kObjectArg))))); +} -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(kInitFunctionCall); +} + +AST_MATCHER_FUNCTION(StatementMatcher, isInitializedFromReferenceToConst) { + auto OldVarDeclRef = + declRefExpr(to(varDecl(hasLocalStorage()).bind(kOldVarDecl))); + 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 the +// isOnlyUsedAsConst() check is sufficient. +// 2. If the (reference or pointer) variable is not initialized in a DeclStmt in +// the BlockStmt. In this case it 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. +bool isInitializingVariableImmutable(const VarDecl &InitializingVar, + const Stmt &BlockStmt, + ASTContext &Context) { + if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context)) { + return false; + } + QualType type = InitializingVar.getType(); + if (!llvm::dyn_cast(type) && + !llvm::dyn_cast(type)) { + // The variable is a value type and we know it is only used as const. Safe + // to reference it and avoid the copy. + return true; + } + auto Matches = + match(findAll(declStmt(has(varDecl(equalsNode(&InitializingVar)))) + .bind("declStmt")), + BlockStmt, Context); + if (Matches.empty()) { + // The reference or pointer is not initialized in the BlockStmt. We assume + // its pointee is not modified then. + return true; + } + const auto *Initialization = selectFirst("declStmt", Matches); + Matches = + match(isInitializedFromReferenceToConst(), *Initialization, Context); + if (selectFirst(kInitFunctionCall, Matches) != nullptr) { + // The reference is initialized from a free function without arguments + // returning a const reference. This is a global immutable object. + return true; + } + if (const auto *OrigVar = selectFirst(kObjectArg, Matches)) { + // Check that the object argument is immutable as well. + return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context); + } + if (const auto *OrigVar = selectFirst(kOldVarDecl, Matches)) { + // Check that the old variable we reference is immutable as well. + return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context); + } + return false; +} + +} // namespace UnnecessaryCopyInitialization::UnnecessaryCopyInitialization( StringRef Name, ClangTidyContext *Context) @@ -41,22 +130,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 +156,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(kOldVarDecl)))), 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(kOldVarDecl); + const auto *ObjectArg = Result.Nodes.getNodeAs(kObjectArg); 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 +189,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 +204,7 @@ if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context)) return; if (ObjectArg != nullptr && - !isOnlyUsedAsConst(*ObjectArg, BlockStmt, Context)) + !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context)) return; auto Diagnostic = @@ -158,7 +224,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,17 @@ auto Copy = Orig.reference(); Update(Copy); } + +void negativeCopiedFromReferenceToModifiedVar() { + ExpensiveToCopyType Orig; + const auto &Ref = Orig; + const auto NecessaryCopy = Ref; + Orig.nonConstMethod(); +} + +void negativeCopiedFromGetterOfReferenceToModifiedVar() { + ExpensiveToCopyType Orig; + const auto &Ref = Orig.reference(); + const auto NecessaryCopy = Ref.reference(); + Orig.nonConstMethod(); +}