diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp index c507043c367a86..5a4c2363bd8af0 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp @@ -75,7 +75,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) { functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()), unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))), has(typeLoc(forEach(ExpensiveValueParamDecl))), - unless(isInstantiated()), decl().bind("functionDecl"))), + decl().bind("functionDecl"))), this); } @@ -133,12 +133,11 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) { // 2. the function is virtual as it might break overrides // 3. the function is referenced outside of a call expression within the // compilation unit as the signature change could introduce build errors. - // 4. the function is a primary template or an explicit template - // specialization. + // 4. the function is an explicit template/ specialization. const auto *Method = llvm::dyn_cast(Function); if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) || isReferencedOutsideOfCallExpr(*Function, *Result.Context) || - (Function->getTemplatedKind() != FunctionDecl::TK_NonTemplate)) + Function->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) return; for (const auto *FunctionDecl = Function; FunctionDecl != nullptr; FunctionDecl = FunctionDecl->getPreviousDecl()) { diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index c1fa502534ea52..930d76f1bf4c42 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -437,6 +437,12 @@ Changes in existing checks Calls to mutable function where there exists a `const` overload are also handled. Fix crash in the case of a non-member operator call. +- Improved :doc:`performance-unnecessary-value-param + ` check + detecting more cases for template functions including lambdas with ``auto``. + E.g., ``std::sort(a.begin(), a.end(), [](auto x, auto y) { return a > b; });`` + will be detected for expensive to copy types. + - Improved :doc:`readability-avoid-return-with-void-value ` check by adding fix-its. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-delayed.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-delayed.cpp index 53ec8713be3389..6a872824896131 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-delayed.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-delayed.cpp @@ -69,7 +69,8 @@ struct PositiveConstValueConstructor { template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) { // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S' - // CHECK-FIXES-NOT: template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) { + // CHECK-MESSAGES: [[@LINE-2]]:95: warning: the parameter 'V' + // CHECK-FIXES: template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, const T& V) { } void instantiated() { diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-templates.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-templates.cpp new file mode 100644 index 00000000000000..688c79bbaa9ac5 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-templates.cpp @@ -0,0 +1,98 @@ +// RUN: %check_clang_tidy -std=c++14-or-later %s performance-unnecessary-value-param %t + +struct ExpensiveToCopyType { + virtual ~ExpensiveToCopyType(); +}; + +template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) { + // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S' + // CHECK-MESSAGES: [[@LINE-2]]:95: warning: the parameter 'V' + // CHECK-FIXES: template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, const T& V) { +} + +void instantiatedWithExpensiveValue() { + templateWithNonTemplatizedParameter( + ExpensiveToCopyType(), ExpensiveToCopyType()); + templateWithNonTemplatizedParameter(ExpensiveToCopyType(), 5); +} + +template void templateWithNonTemplatizedParameterCheapTemplate(const ExpensiveToCopyType S, T V) { + // CHECK-MESSAGES: [[@LINE-1]]:103: warning: the const qualified parameter 'S' + // CHECK-FIXES: template void templateWithNonTemplatizedParameterCheapTemplate(const ExpensiveToCopyType& S, T V) { +} + +void instantiatedWithCheapValue() { + templateWithNonTemplatizedParameterCheapTemplate(ExpensiveToCopyType(), 5); +} + +template void nonInstantiatedTemplateWithConstValue(const T S) {} +template void nonInstantiatedTemplateWithNonConstValue(T S) {} + +template void instantiatedTemplateSpecialization(T NoSpecS) {} +template <> void instantiatedTemplateSpecialization(int SpecSInt) {} +// Updating template specialization would also require to update the main +// template and other specializations. Such specializations may be +// spreaded across different translation units. +// For that reason we only issue a warning, but do not propose fixes. +template <> +void instantiatedTemplateSpecialization( + ExpensiveToCopyType SpecSExpensiveToCopy) { + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: the parameter 'SpecSExpensiveToCopy' + // CHECK-FIXES-NOT: const T& NoSpecS + // CHECK-FIXES-NOT: const int& SpecSInt + // CHECK-FIXES-NOT: const ExpensiveToCopyType& SpecSExpensiveToCopy +} + +void instantiatedTemplateSpecialization() { + instantiatedTemplateSpecialization(ExpensiveToCopyType()); +} + +template void instantiatedTemplateWithConstValue(const T S) { + // CHECK-MESSAGES: [[@LINE-1]]:71: warning: the const qualified parameter 'S' + // CHECK-FIXES: template void instantiatedTemplateWithConstValue(const T& S) { +} + +void instantiatedConstValue() { + instantiatedTemplateWithConstValue(ExpensiveToCopyType()); +} + +template void instantiatedTemplateWithNonConstValue(T S) { + // CHECK-MESSAGES: [[@LINE-1]]:68: warning: the parameter 'S' + // CHECK-FIXES: template void instantiatedTemplateWithNonConstValue(const T& S) { +} + +void instantiatedNonConstValue() { + instantiatedTemplateWithNonConstValue(ExpensiveToCopyType()); +} + +void lambdaConstValue() { + auto fn = [](const ExpensiveToCopyType S) { + // CHECK-MESSAGES: [[@LINE-1]]:42: warning: the const qualified parameter 'S' + // CHECK-FIXES: auto fn = [](const ExpensiveToCopyType& S) { + }; + fn(ExpensiveToCopyType()); +} + +void lambdaNonConstValue() { + auto fn = [](ExpensiveToCopyType S) { + // CHECK-MESSAGES: [[@LINE-1]]:36: warning: the parameter 'S' + // CHECK-FIXES: auto fn = [](const ExpensiveToCopyType& S) { + }; + fn(ExpensiveToCopyType()); +} + +void lambdaConstAutoValue() { + auto fn = [](const auto S) { + // CHECK-MESSAGES: [[@LINE-1]]:27: warning: the const qualified parameter 'S' + // CHECK-FIXES: auto fn = [](const auto& S) { + }; + fn(ExpensiveToCopyType()); +} + +void lambdaNonConstAutoValue() { + auto fn = [](auto S) { + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: the parameter 'S' + // CHECK-FIXES: auto fn = [](const auto& S) { + }; + fn(ExpensiveToCopyType()); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp index d578eedd94a390..0dffaefa213a45 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp @@ -107,19 +107,6 @@ struct PositiveConstValueConstructor { // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {} }; -template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) { - // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S' - // CHECK-FIXES-NOT: template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) { -} - -void instantiated() { - templateWithNonTemplatizedParameter(ExpensiveToCopyType(), ExpensiveToCopyType()); - templateWithNonTemplatizedParameter(ExpensiveToCopyType(), 5); -} - -template void negativeTemplateType(const T V) { -} - void negativeArray(const ExpensiveToCopyType[]) { } @@ -370,35 +357,3 @@ void fun() { ExpensiveToCopyType E; NegativeUsingConstructor S(E); } - -template -void templateFunction(T) { -} - -template<> -void templateFunction(ExpensiveToCopyType E) { - // CHECK-MESSAGES: [[@LINE-1]]:64: warning: the parameter 'E' is copied - // CHECK-FIXES: void templateFunction(ExpensiveToCopyType E) { - E.constReference(); -} - -template -T templateSpecializationFunction(ExpensiveToCopyType E) { - // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied - // CHECK-FIXES-NOT: T templateSpecializationFunction(const ExpensiveToCopyType& E) { - return T(); -} - -template <> -bool templateSpecializationFunction(ExpensiveToCopyType E) { - // CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied - // CHECK-FIXES-NOT: bool templateSpecializationFunction(const ExpensiveToCopyType& E) { - return true; -} - -template <> -int templateSpecializationFunction(ExpensiveToCopyType E) { - // CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied - // CHECK-FIXES-NOT: int templateSpecializationFunction(const ExpensiveToCopyType& E) { - return 0; -}