diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h @@ -43,6 +43,7 @@ const Stmt &BlockStmt, const DeclStmt &Stmt, bool IssueFix, ASTContext &Context); const std::vector AllowedTypes; + const std::vector ExcludedContainerTypes; }; } // namespace performance 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 @@ -75,7 +75,8 @@ } } -AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) { +AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall, + std::vector, ExcludedContainerTypes) { // 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 @@ -85,7 +86,11 @@ return cxxMemberCallExpr( callee(cxxMethodDecl(returns(matchers::isReferenceToConst())) .bind(MethodDeclId)), - on(declRefExpr(to(varDecl().bind(ObjectArgId))))); + on(declRefExpr(to( + varDecl( + unless(hasType(qualType(hasCanonicalType(hasDeclaration(namedDecl( + matchers::matchesAnyListedName(ExcludedContainerTypes)))))))) + .bind(ObjectArgId))))); } AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) { @@ -98,11 +103,13 @@ .bind(InitFunctionCallId); } -AST_MATCHER_FUNCTION(StatementMatcher, initializerReturnsReferenceToConst) { +AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst, + std::vector, ExcludedContainerTypes) { auto OldVarDeclRef = declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId))); return expr( - anyOf(isConstRefReturningFunctionCall(), isConstRefReturningMethodCall(), + anyOf(isConstRefReturningFunctionCall(), + isConstRefReturningMethodCall(ExcludedContainerTypes), ignoringImpCasts(OldVarDeclRef), ignoringImpCasts(unaryOperator(hasOperatorName("&"), hasUnaryOperand(OldVarDeclRef))))); @@ -120,9 +127,9 @@ // 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) { +static bool isInitializingVariableImmutable( + const VarDecl &InitializingVar, const Stmt &BlockStmt, ASTContext &Context, + const std::vector &ExcludedContainerTypes) { if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context)) return false; @@ -138,18 +145,21 @@ return true; } - auto Matches = match(initializerReturnsReferenceToConst(), - *InitializingVar.getInit(), Context); + auto Matches = + match(initializerReturnsReferenceToConst(ExcludedContainerTypes), + *InitializingVar.getInit(), 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); + return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context, + ExcludedContainerTypes); // Check that the old variable we reference is immutable as well. if (const auto *OrigVar = selectFirst(OldVarDeclId, Matches)) - return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context); + return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context, + ExcludedContainerTypes); return false; } @@ -204,7 +214,9 @@ StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), AllowedTypes( - utils::options::parseStringList(Options.get("AllowedTypes", ""))) {} + utils::options::parseStringList(Options.get("AllowedTypes", ""))), + ExcludedContainerTypes(utils::options::parseStringList( + Options.get("ExcludedContainerTypes", ""))) {} void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) { auto LocalVarCopiedFrom = [this](const internal::Matcher &CopyCtorArg) { @@ -235,7 +247,8 @@ }; Finder->addMatcher(LocalVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(), - isConstRefReturningMethodCall())), + isConstRefReturningMethodCall( + ExcludedContainerTypes))), this); Finder->addMatcher(LocalVarCopiedFrom(declRefExpr( @@ -291,7 +304,8 @@ if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context)) return; if (ObjectArg != nullptr && - !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context)) + !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context, + ExcludedContainerTypes)) return; if (isVariableUnused(Var, BlockStmt, Context)) { auto Diagnostic = @@ -318,7 +332,8 @@ const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt, const DeclStmt &Stmt, bool IssueFix, ASTContext &Context) { if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) || - !isInitializingVariableImmutable(OldVar, BlockStmt, Context)) + !isInitializingVariableImmutable(OldVar, BlockStmt, Context, + ExcludedContainerTypes)) return; if (isVariableUnused(NewVar, BlockStmt, Context)) { @@ -344,6 +359,8 @@ ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "AllowedTypes", utils::options::serializeStringList(AllowedTypes)); + Options.store(Opts, "ExcludedContainerTypes", + utils::options::serializeStringList(ExcludedContainerTypes)); } } // namespace performance diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst --- a/clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst @@ -47,3 +47,15 @@ is empty. If a name in the list contains the sequence `::` it is matched against the qualified typename (i.e. `namespace::Type`, otherwise it is matched against only the type name (i.e. `Type`). + +.. option:: ExcludedContainerTypes + + A semicolon-separated list of names of types whose methods are allowed to + return the const reference the variable is copied from. When an expensive to + copy variable is copy initialized by the return value from a type on this + list the check does not trigger. This can be used to exclude types known to + be const incorrect or where the lifetime or immutability of returned + references is not tied to mutations of the container. An example are view + types that don't own the underlying data. Like for `AllowedTypes` above, + regular expressions are accepted and the inclusion of `::` determines whether + the qualified typename is matched or not. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp @@ -0,0 +1,60 @@ +// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t -- -config="{CheckOptions: [{key: performance-unnecessary-copy-initialization.ExcludedContainerTypes, value: 'ns::ViewType$;::ConstInCorrectType$'}]}" -- + +namespace ns { +template +struct ViewType { + ViewType(const T &); + const T &view() const; +}; +} // namespace ns + +template +struct ViewType { + ViewType(const T &); + const T &view() const; +}; + +struct ExpensiveToCopy { + ~ExpensiveToCopy(); + void constMethod() const; +}; + +struct ConstInCorrectType { + const ExpensiveToCopy &secretlyMutates() const; +}; + +using NSVTE = ns::ViewType; +typedef ns::ViewType FewType; + +void positiveViewType() { + ExpensiveToCopy E; + ViewType V(E); + const auto O = V.view(); + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'O' is copy-constructed + // CHECK-FIXES: const auto& O = V.view(); + O.constMethod(); +} + +void excludedViewTypeInNamespace() { + ExpensiveToCopy E; + ns::ViewType V(E); + const auto O = V.view(); + O.constMethod(); +} + +void excludedViewTypeAliased() { + ExpensiveToCopy E; + NSVTE V(E); + const auto O = V.view(); + O.constMethod(); + + FewType F(E); + const auto P = F.view(); + P.constMethod(); +} + +void excludedConstIncorrectType() { + ConstInCorrectType C; + const auto E = C.secretlyMutates(); + E.constMethod(); +}