Author: Felix Berger Date: 2021-07-22T16:20:20-04:00 New Revision: cb4c12b6117a6f2989d5745854a94c75cb6a09ba
URL: https://github.com/llvm/llvm-project/commit/cb4c12b6117a6f2989d5745854a94c75cb6a09ba DIFF: https://github.com/llvm/llvm-project/commit/cb4c12b6117a6f2989d5745854a94c75cb6a09ba.diff LOG: [clang-tidy] performance-unnecessary-copy-initialization: Create option to exclude container types from triggering the check. Add string list option of type names analagous to `AllowedTypes` which lets users specify a list of ExcludedContainerTypes. Types matching this list will not trigger the check when an expensive variable is copy initialized from a const accessor method they provide, i.e.: ``` ExcludedContainerTypes = 'ExcludedType' void foo() { ExcludedType<ExpensiveToCopy> Container; const ExpensiveToCopy NecessaryCopy = Container.get(); } ``` Even though an expensive to copy variable is copy initialized the check does not trigger because the container type is excluded. This is useful for container types that don't own their data, such as view types where modification of the returned references in other places cannot be reliably tracked, or const incorrect types. Differential Revision: https://reviews.llvm.org/D106173 Reviewed-by: ymandel Added: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp Modified: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp index 9631e06a8f95b..6f9495fd1e66c 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp @@ -75,7 +75,8 @@ void recordRemoval(const DeclStmt &Stmt, ASTContext &Context, } } -AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) { +AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall, + std::vector<std::string>, 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 @@ AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) { 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 @@ AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) { .bind(InitFunctionCallId); } -AST_MATCHER_FUNCTION(StatementMatcher, initializerReturnsReferenceToConst) { +AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst, + std::vector<std::string>, 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 @@ AST_MATCHER_FUNCTION(StatementMatcher, initializerReturnsReferenceToConst) { // 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<std::string> &ExcludedContainerTypes) { if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context)) return false; @@ -138,18 +145,21 @@ static bool isInitializingVariableImmutable(const VarDecl &InitializingVar, 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<CallExpr>(InitFunctionCallId, Matches) != nullptr) return true; // Check that the object argument is immutable as well. if (const auto *OrigVar = selectFirst<VarDecl>(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<VarDecl>(OldVarDeclId, Matches)) - return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context); + return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context, + ExcludedContainerTypes); return false; } @@ -204,7 +214,9 @@ UnnecessaryCopyInitialization::UnnecessaryCopyInitialization( 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<Expr> &CopyCtorArg) { @@ -235,7 +247,8 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) { }; Finder->addMatcher(LocalVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(), - isConstRefReturningMethodCall())), + isConstRefReturningMethodCall( + ExcludedContainerTypes))), this); Finder->addMatcher(LocalVarCopiedFrom(declRefExpr( @@ -291,7 +304,8 @@ void UnnecessaryCopyInitialization::handleCopyFromMethodReturn( 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 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar( 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 @@ void UnnecessaryCopyInitialization::storeOptions( 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/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h index c11be2d8885d6..d81a89aa3e559 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h @@ -43,6 +43,7 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck { const Stmt &BlockStmt, const DeclStmt &Stmt, bool IssueFix, ASTContext &Context); const std::vector<std::string> AllowedTypes; + const std::vector<std::string> 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 index 7a8fe8408f422..837283811ddcc 100644 --- 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 @@ Options 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 index 0000000000000..6d1d28ad14985 --- /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 <typename T> +struct ViewType { + ViewType(const T &); + const T &view() const; +}; +} // namespace ns + +template <typename T> +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<ExpensiveToCopy>; +typedef ns::ViewType<ExpensiveToCopy> FewType; + +void positiveViewType() { + ExpensiveToCopy E; + ViewType<ExpensiveToCopy> 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<ExpensiveToCopy> 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(); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
