Index: clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt +++ clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt @@ -6,6 +6,7 @@ ImplicitCastInLoopCheck.cpp PerformanceTidyModule.cpp UnnecessaryCopyInitialization.cpp + UnnecessaryValueParamCheck.cpp LINK_LIBS clangAST Index: clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp +++ clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp @@ -15,6 +15,7 @@ #include "ForRangeCopyCheck.h" #include "ImplicitCastInLoopCheck.h" #include "UnnecessaryCopyInitialization.h" +#include "UnnecessaryValueParamCheck.h" namespace clang { namespace tidy { @@ -31,6 +32,8 @@ "performance-implicit-cast-in-loop"); CheckFactories.registerCheck( "performance-unnecessary-copy-initialization"); + CheckFactories.registerCheck( + "performance-unnecessary-value-param"); } }; Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h +++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h @@ -0,0 +1,36 @@ +//===--- UnnecessaryValueParamCheck.h - clang-tidy---------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_VALUE_PARAM_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_VALUE_PARAM_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace performance { + +/// \brief A check that flags value parameters of expensive to copy types that +/// can safely be converted to const references. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html +class UnnecessaryValueParamCheck : public ClangTidyCheck { +public: + UnnecessaryValueParamCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace performance +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_VALUE_PARAM_H Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp @@ -0,0 +1,87 @@ +//===--- UnnecessaryValueParamCheck.cpp - clang-tidy-----------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "UnnecessaryValueParamCheck.h" + +#include "../utils/DeclRefExprUtils.h" +#include "../utils/FixItHintUtils.h" +#include "../utils/Matchers.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace performance { + +namespace { + +std::string paramNameOrIndex(StringRef Name, size_t Index) { + return (Name.empty() ? llvm::Twine('#') + llvm::Twine(Index + 1) + : llvm::Twine('\'') + Name + llvm::Twine('\'')) + .str(); +} + +} // namespace + +void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) { + const auto ExpensiveValueParamDecl = + parmVarDecl(hasType(hasCanonicalType(allOf(matchers::isExpensiveToCopy(), + unless(referenceType())))), + decl().bind("param")); + Finder->addMatcher( + functionDecl(isDefinition(), unless(cxxMethodDecl(isOverride())), + unless(isInstantiated()), + has(typeLoc(forEach(ExpensiveValueParamDecl))), + decl().bind("functionDecl")), + this); +} + +void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Param = Result.Nodes.getNodeAs("param"); + const auto *Function = Result.Nodes.getNodeAs("functionDecl"); + const size_t Index = std::find(Function->parameters().begin(), + Function->parameters().end(), Param) - + Function->parameters().begin(); + bool IsConstQualified = + Param->getType().getCanonicalType().isConstQualified(); + + // Do not trigger on non-const value parameters when: + // 1. they are in a constructor definition since they can likely trigger + // misc-move-constructor-init which will suggest to move the argument. + // 2. they are not only used as const. + if (!IsConstQualified && (llvm::isa(Function) || + !Function->doesThisDeclarationHaveABody() || + !decl_ref_expr_utils::isOnlyUsedAsConst( + *Param, *Function->getBody(), *Result.Context))) + return; + auto Diag = + diag(Param->getLocation(), + IsConstQualified ? "the const qualified parameter %0 is " + "copied for each invocation; consider " + "making it a reference" + : "the parameter %0 is copied for each " + "invocation but only used as a const reference; " + "consider making it a const reference") + << paramNameOrIndex(Param->getName(), Index); + // Do not propose fixes in macros since we cannot place them correctly. + if (Param->getLocStart().isMacroID()) + return; + for (const auto *FunctionDecl = Function; FunctionDecl != nullptr; + FunctionDecl = FunctionDecl->getPreviousDecl()) { + const auto &CurrentParam = *FunctionDecl->getParamDecl(Index); + Diag << utils::create_fix_it::changeVarDeclToReference(CurrentParam, + *Result.Context); + if (!IsConstQualified) + Diag << utils::create_fix_it::changeVarDeclToConst(CurrentParam); + } +} + +} // namespace performance +} // namespace tidy +} // namespace clang Index: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst @@ -89,6 +89,7 @@ performance-faster-string-find performance-for-range-copy performance-implicit-cast-in-loop + performance-unnecessary-value-param performance-unnecessary-copy-initialization readability-braces-around-statements readability-container-size-empty Index: clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst @@ -0,0 +1,33 @@ +.. title:: clang-tidy - performance-unnecessary-value-param + +performance-unnecessary-value-param +=================================== + +Flags value parameter declarations of expensive to copy types that are copied +for each invocation but it would suffice to pass them by const reference. + +The check is only applied to parameters of types that are expensive to copy +which means they are not trivially copyable or have a non-trivial copy +constructor or destructor. + +To ensure that it is safe to replace the value paramater with a const reference +the following heuristic is employed: + +1. the parameter is const qualified; +2. the parameter is not const, but only const methods or operators are invoked + on it, or it is used as const reference or value argument in constructors or + function calls. + +Example: + +.. code-block:: c++ + + void f(const string Value) { + // The warning will suggest making Value a reference. + } + + void g(ExpensiveToCopy Value) { + // The warning will suggest making Value a const reference. + Value.ConstMethd(); + ExpensiveToCopy Copy(Value); + } Index: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp +++ clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp @@ -0,0 +1,171 @@ +// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t + +struct ExpensiveToCopyType { + const ExpensiveToCopyType & constReference() const { + return *this; + } + void nonConstMethod(); + virtual ~ExpensiveToCopyType(); +}; + +void mutate(ExpensiveToCopyType &); +void mutate(ExpensiveToCopyType *); +void useAsConstReference(const ExpensiveToCopyType &); +void useByValue(ExpensiveToCopyType); + +// This class simulates std::pair<>. It is trivially copy constructible +// and trivially destructible, but not trivially copy assignable. +class SomewhatTrivial { + public: + SomewhatTrivial(); + SomewhatTrivial(const SomewhatTrivial&) = default; + ~SomewhatTrivial() = default; + SomewhatTrivial& operator=(const SomewhatTrivial&); +}; + +void positiveExpensiveConstValue(const ExpensiveToCopyType Obj); +// CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj); +void positiveExpensiveConstValue(const ExpensiveToCopyType Obj) { + // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'Obj' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param] + // CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj) { +} + +void positiveExpensiveValue(ExpensiveToCopyType Obj); +// CHECK-FIXES: void positiveExpensiveValue(const ExpensiveToCopyType& Obj); +void positiveExpensiveValue(ExpensiveToCopyType Obj) { + // CHECK-MESSAGES: [[@LINE-1]]:49: warning: the parameter 'Obj' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param] + // CHECK-FIXES: void positiveExpensiveValue(const ExpensiveToCopyType& Obj) { + Obj.constReference(); + useAsConstReference(Obj); + auto Copy = Obj; + useByValue(Obj); +} + +void positiveWithComment(const ExpensiveToCopyType /* important */ S); +// CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S); +void positiveWithComment(const ExpensiveToCopyType /* important */ S) { + // CHECK-MESSAGES: [[@LINE-1]]:68: warning: the const qualified + // CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S) { +} + +void positiveUnnamedParam(const ExpensiveToCopyType) { + // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the const qualified parameter #1 + // CHECK-FIXES: void positiveUnnamedParam(const ExpensiveToCopyType&) { +} + +void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToCopyType& ConstRef, ExpensiveToCopyType Copy); +// CHECK-FIXES: void positiveAndNegative(const ExpensiveToCopyType& ConstCopy, const ExpensiveToCopyType& ConstRef, const ExpensiveToCopyType& Copy); +void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToCopyType& ConstRef, ExpensiveToCopyType Copy) { + // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the const qualified parameter 'ConstCopy' + // CHECK-MESSAGES: [[@LINE-2]]:120: warning: the parameter 'Copy' + // CHECK-FIXES: void positiveAndNegative(const ExpensiveToCopyType& ConstCopy, const ExpensiveToCopyType& ConstRef, const ExpensiveToCopyType& Copy) { +} + +struct PositiveConstValueConstructor { + PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {} + // CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy' +}; + +template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) { + // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S' + // CHECK-FIXES: 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[]) { +} + +void negativePointer(ExpensiveToCopyType* Obj) { +} + +void negativeConstPointer(const ExpensiveToCopyType* Obj) { +} + +void negativeConstReference(const ExpensiveToCopyType& Obj) { +} + +void negativeReference(ExpensiveToCopyType& Obj) { +} + +void negativeUniversalReference(ExpensiveToCopyType&& Obj) { +} + +void negativeSomewhatTrivialConstValue(const SomewhatTrivial Somewhat) { +} + +void negativeSomewhatTrivialValue(SomewhatTrivial Somewhat) { +} + +void negativeConstBuiltIn(const int I) { +} + +void negativeValueBuiltIn(int I) { +} + +void negativeValueIsMutatedByReference(ExpensiveToCopyType Obj) { + mutate(Obj); +} + +void negativeValueIsMutatatedByPointer(ExpensiveToCopyType Obj) { + mutate(&Obj); +} + +void negativeValueIsReassigned(ExpensiveToCopyType Obj) { + Obj = ExpensiveToCopyType(); +} + +void negativeValueNonConstMethodIsCalled(ExpensiveToCopyType Obj) { + Obj.nonConstMethod(); +} + +struct NegativeValueCopyConstructor { + NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {} +}; + +template +struct Container { + typedef const T & const_reference; +}; + +void NegativeTypedefParam(const Container::const_reference Param) { +} + +#define UNNECESSARY_VALUE_PARAM_IN_MACRO_BODY() \ + void inMacro(const ExpensiveToCopyType T) { \ + } \ +// Ensure fix is not applied. +// CHECK-FIXES: void inMacro(const ExpensiveToCopyType T) { + +UNNECESSARY_VALUE_PARAM_IN_MACRO_BODY() +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: the const qualified parameter 'T' + +#define UNNECESSARY_VALUE_PARAM_IN_MACRO_ARGUMENT(ARGUMENT) \ + ARGUMENT + +UNNECESSARY_VALUE_PARAM_IN_MACRO_ARGUMENT(void inMacroArgument(const ExpensiveToCopyType InMacroArg) {}) +// CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'InMacroArg' +// CHECK-FIXES: void inMacroArgument(const ExpensiveToCopyType InMacroArg) {} + +struct VirtualMethod { + virtual ~VirtualMethod() {} + virtual void handle(ExpensiveToCopyType T) const = 0; +}; + +struct NegativeOverriddenMethod : public VirtualMethod { + void handle(ExpensiveToCopyType Overridden) const { + // CHECK-FIXES: handle(ExpensiveToCopyType Overridden) const { + } +}; + +struct NegativeDeletedMethod { + ~NegativeDeletedMethod() {} + NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete; + // CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete; +};