diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -4,6 +4,7 @@ ) add_clang_library(clangTidyPerformanceModule + ConstParameterValueOrRef.cpp FasterStringFindCheck.cpp ForRangeCopyCheck.cpp ImplicitConversionInLoopCheck.cpp diff --git a/clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h b/clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h @@ -0,0 +1,50 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_CONSTPARAMETERVALUEORREF_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_CONSTPARAMETERVALUEORREF_H + +#include "../ClangTidyCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +namespace clang { +namespace tidy { +namespace performance { +// Checks ``const``-qualified parameters to determine if they should be passed +// by value or by reference-to-``const`` for function definitions. Ignores +// proper function declarations, parameters without ``const`` in their type +// specifier, and dependent types. +class ConstParameterValueOrRef : public ClangTidyCheck { +public: + using ClangTidyCheck::ClangTidyCheck; + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } + + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + Optional MaxSize; + + void checkPassByValueIsAppropriate(const ParmVarDecl &Param, + const FunctionDecl &Func, + const ASTContext &Context); + void checkPassByRefIsAppropriate(const ParmVarDecl &Param, + const ASTContext &Context); +}; + +struct MaxSize { + int Size; +}; +} // namespace performance +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_CONSTPARAMETERVALUEORREF_H diff --git a/clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp b/clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp @@ -0,0 +1,135 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "ConstParameterValueOrRef.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/Diagnostic.h" +#include "llvm/ADT/ArrayRef.h" +#include + +namespace clang { +namespace tidy { +namespace performance { + +using ast_matchers::allOf; +using ast_matchers::forEach; +using ast_matchers::functionDecl; +using ast_matchers::has; +using ast_matchers::hasType; +using ast_matchers::isConstQualified; +using ast_matchers::isDefinition; +using ast_matchers::isTemplateInstantiation; +using ast_matchers::MatchFinder; +using ast_matchers::parmVarDecl; +using ast_matchers::references; +using ast_matchers::typeLoc; +using ast_matchers::unless; + +void ConstParameterValueOrRef::registerMatchers(MatchFinder *Finder) { + const auto ConstValueParamDecl = + parmVarDecl(hasType(isConstQualified())).bind("value"); + Finder->addMatcher( + functionDecl(allOf(isDefinition(), + has(typeLoc(forEach(ConstValueParamDecl))), + unless(isTemplateInstantiation()))) + .bind("func-value"), + this); + + const auto RefToConstParamDecl = + parmVarDecl(hasType(references(isConstQualified()))).bind("ref"); + Finder->addMatcher( + functionDecl( + allOf(isDefinition(), has(typeLoc(forEach(RefToConstParamDecl)))), + unless(isTemplateInstantiation())) + .bind("func-ref"), + this); +} + +namespace { +bool isSharedPtr(const QualType &T) { + if (auto R = T->getAsCXXRecordDecl()) + return R->getQualifiedNameAsString() == "std::shared_ptr" || + R->getQualifiedNameAsString() == "boost::shared_ptr"; + return false; +} +} // namespace + +void ConstParameterValueOrRef::checkPassByValueIsAppropriate( + const ParmVarDecl &Param, const FunctionDecl &Func, + const ASTContext &Context) { + if (!Func.isFirstDecl()) + return; + + const QualType &T = Param.getType(); + + if (isSharedPtr(T)) + return; + + Optional Size = Context.getTypeSizeInCharsIfKnown(T); + if (!Size.hasValue()) + return; + + SourceLocation Loc = Param.getLocation(); + FixItHint Hint = FixItHint::CreateInsertion(Loc, "&"); + if (Size->getQuantity() > *MaxSize) { + diag(Loc, "%0 is a large type: pass by reference-to-const instead") + << T << Hint; + return; + } + + if (!T.isTriviallyCopyableType(Context)) { + diag(Loc, + "%0 is not trivially-copyable: pass by reference-to-const instead") + << T << Hint; + return; + } +} + +void ConstParameterValueOrRef::checkPassByRefIsAppropriate( + const ParmVarDecl &Param, const ASTContext &Context) { + const QualType &T = Param.getType().getNonReferenceType(); + + Optional Size = Context.getTypeSizeInCharsIfKnown(T); + if (!Size.hasValue()) + return; + + if (isSharedPtr(T)) + return; + + if (Size->getQuantity() <= *MaxSize && T.isTriviallyCopyableType(Context)) { + diag(Param.getTypeSpecEndLoc(), + "%0 is a small, trivially-copyable type: pass by value instead") + << T << FixItHint::CreateRemoval(Param.getTypeSpecEndLoc()); + return; + } +} + +void ConstParameterValueOrRef::check(const MatchFinder::MatchResult &Result) { + if (!MaxSize.hasValue()) { + // If the user hasn't provided a default, set the default to sixteen bytes, + // which is the largest power-of-two size before performance is impacted + // across multiple chipsets. See user-facing documentation for more info. + static constexpr int DefaultMaxSize = 16; + MaxSize = DefaultMaxSize; + } + + if (const auto *Param = Result.Nodes.getNodeAs("value")) { + const auto *Func = Result.Nodes.getNodeAs("func-value"); + assert(Func != nullptr); + return checkPassByValueIsAppropriate(*Param, *Func, *Result.Context); + } + + if (const auto *Param = Result.Nodes.getNodeAs("ref")) + return checkPassByRefIsAppropriate(*Param, *Result.Context); +} +} // namespace performance +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -9,6 +9,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "ConstParameterValueOrRef.h" #include "FasterStringFindCheck.h" #include "ForRangeCopyCheck.h" #include "ImplicitConversionInLoopCheck.h" @@ -32,6 +33,8 @@ class PerformanceModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "performance-const-parameter-value-or-ref"); CheckFactories.registerCheck( "performance-faster-string-find"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst @@ -0,0 +1,125 @@ +.. title:: clang-tidy - performance-const-parameter-value-or-ref + +performance-const-parameter-value-or-ref +======================================== + +Checks ``const``-qualified parameters to determine if they should be passed by +value or by reference-to-``const`` for function definitions. Ignores proper +function declarations, parameters without ``const`` in their type specifier, and +dependent types. + +A type is recommended to be passed by value if it is both "small" and trivially +copyable; otherwise the type recommendation flips to being pass by +reference-to-``const``. + +This check works well with ``readability-avoid-const-params-in-decls``. + + +Examples +-------- + +.. code-block:: c++ + + // No warnings for proper function declarations + void f1(const int &); + void f1(const std::string); // use readability-avoid-const-params-in-decls + + // No warnings when value parameter is non-const + void f2(int i) {} + void f2(std::string s) {} + + // No warnings when passing by value and parameter is "small" + trivially + // copyable + void f3(const char c) {} + void f3(const std::array a) {} + + // No warning when passing by reference-to-const and parameter is not "small" + void f4(const std::array &a) {} + + // No warning when passing by reference-to-const and type isn't trivially + // copyable + void f5(const std::string &) {} + + void f6(int const& i) {} + // warning: 'const int' is a small, trivially copyable type: pass by value instead + // fix-it: void f6(const int i) {} + + void f7(const std::array a) {} + // warning: 'const std::array' is a large type: pass by reference-to-const instead + // fix-it: void f7(const std::array &a) {} + + void f8(const std::string s) {} + // warning: 'const std::string' is not trivially copyable: pass by reference-to-const instead + + void f9(const std::shared_ptr) {} // no warning by default: see below + void f10(const std::shared_ptr &) {} // no warning by default: see below + +Options +------- + +.. option:: SmallMaxSize + +Positive integer that indicates the largest size an object can be while being +passed by ``const``-qualified value. The default value is 32 bytes. + +.. option:: EnableSharedPtrWarningFor + +**Accepted values**: ``'ConstValue'``, ``'RefToConst'``, ``Neither`` + +Directs clang-tidy to warn about ``std::shared_ptr`` and ``boost::shared_ptr`` +on a particular condition. + + +Design +------ + +Liberal use of ``const`` is good: it improves the integrity of a program and +gives readers confidence that certain objects won't be modified throughout their +lifetime. To mitigate performance bugs with respect to unnecessarily passing by +value without compromising the aforementioned integrity, this check performs two +tests: a ``const``-qualified value parameter is allowed if the programmer +considers the type to be small, and if the type is trivially copyable. + +A type that's not trivially copyable is difficult to reason about at a glance. +Well-designed types usually use copy constructors and copy-assignment operators +to allocate new resources and copy the data of the other object in some +non-constant time fashion. There are a few exceptions to this rule, such as +``std::shared_ptr``, which often needs to be passed by value to increase the +reference count (but can be marked as ``const`` to prevent releasing, etc.). +Passing a ``shared_ptr`` by reference-to-``const`` is also acceptable in certain +situaitons. As such, ``std::shared_ptr`` and ``boost::shared_ptr`` will +not warn in either case by default. + +If a function has a visible forward declaration, then the check will ignore a +const-qualified value parameter that's ``const``-qualified. + +.. code-block:: c++ + // No warning because there's a forward declaration + void f(std::vector); + void f(const std::vector) {} + +The default value for ``SmallMaxSize`` was determined by through benchmarking +when passing by value was no longer competetive with passing by reference for +various `boards `_. The benchmark can be found on +`Compiler Explorer `_, with the used to inform +the threshold. + + +Limitations and future work +--------------------------- + +The forward declaration approach to asserting to the compiler that a parameter +is intended to be passed by value is only applicable to free functions and +member functions defined out-of-line. Inline member functions, hidden friends, +and lambdas miss out on this in-source warning suppression. + +This check only supports ``std::shared_ptr`` and ``boost::shared_ptr`` as +flexible types. Future work will establish a way for users to declare +non-trivially copyable types that can be flexible. + +Template type parameters types aren't yet supported. It isn't clear how to +support type parameters types in the general case (especially without proper +static analysis), but there may be a way to set up mnemonics for clang-tidy to +follow when using concepts. For example, a type parameter requiring +``std::integral`` is always an integer, so it might be possible to use this +knowledge to our advantage. Future work will look into this. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-const-parameter-value-or-ref.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-const-parameter-value-or-ref.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-const-parameter-value-or-ref.cpp @@ -0,0 +1,260 @@ +// RUN: %check_clang_tidy %s performance-const-parameter-value-or-ref %t + +template +struct array { + T base[N]; +}; + +using MaxSmall = array; +using MinLarge = array; + +class NotTriviallyCopyable { +public: + NotTriviallyCopyable() = default; + + NotTriviallyCopyable(const NotTriviallyCopyable &) {} +}; + +// No warnings for forward declarations: this is the domain of +// readability-avoid-const-params-in-decls +void f1(const __int128); +void f1(const MaxSmall); +void f1(const MinLarge); +void f1(const NotTriviallyCopyable); + +struct S1 { + void f(const __int128); + void f(const MaxSmall); + void f(const MinLarge); + void f(const NotTriviallyCopyable); +}; + +void f2(const __int128 &); +void f2(const MaxSmall &); +void f2(const MinLarge &); +void f2(const NotTriviallyCopyable &); +void f2(const array &); + +struct S2 { + void f(const __int128 &); + void f(const MaxSmall &); + void f(const MinLarge &); + void f(const NotTriviallyCopyable &); + void f(const array &); +}; + +// No warnings for function definitions that have been forward declared (pass +// by value only). +void f1(const MinLarge) {} +void f1(const NotTriviallyCopyable) {} + +// No warnings when passing by value + mutable parameter +void f3(__int128) {} +void f3(MaxSmall) {} +void f3(MinLarge) {} +void f3(NotTriviallyCopyable) {} +void f3(array) {} + +struct S3 { + void f(__int128) {} + void f(MaxSmall) {} + void f(MinLarge) {} + void f(NotTriviallyCopyable) {} + void f(array) {} +}; + +// No warnings when passing by value + "small" parameter +void f4(const __int128) {} +void f4(const MaxSmall) {} + +struct S4 { + void f(const __int128) {} + void f(const MaxSmall) {} +}; + +// No warnings when passing by reference-to-const + "large"/non-trivial parameter +void f5(const MinLarge &) {} +void f5(const NotTriviallyCopyable &) {} +void f5(const array &) {} + +struct S5 { + void f(const MinLarge &) {} + void f(const NotTriviallyCopyable &) {} + void f(const array &) {} +}; + +// No warnings for dependent types +template +T f6(const T t) { return t; } + +const auto R1 = f6(false); +const auto R2 = f6(MaxSmall()); +const auto R3 = f6(MinLarge()); +const auto R4 = f6(NotTriviallyCopyable()); + +struct S6 { + template + T f(const T t) { return t; } + + S6() { + const auto R1 = f(false); + const auto R2 = f(MaxSmall()); + const auto R3 = f(MinLarge()); + const auto R4 = f(NotTriviallyCopyable()); + } +}; + +template +T f7(const T &t) { return t; } + +const auto R5 = f7(false); +const auto R6 = f7(MaxSmall()); +const auto R7 = f7(MinLarge()); +const auto R8 = f7(NotTriviallyCopyable()); + +struct S7 { + template + T f(const T &t) { return t; } + + S7() { + const auto R5 = f(false); + const auto R6 = f(MaxSmall()); + const auto R7 = f(MinLarge()); + const auto R8 = f(NotTriviallyCopyable()); + } +}; + +//////////////////////////////////////////////////////////////////////////////// +// Warn when passing a "small" parameter by reference-to-const. +//////////////////////////////////////////////////////////////////////////////// +void f2(const __int128 &) {} +// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'const __int128' is a small, trivially-copyable type: pass by value instead +// CHECK-FIXES: void f2(const __int128) {} + +void f8(const __int128 &) {} +// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'const __int128' is a small, trivially-copyable type: pass by value instead +// CHECK-FIXES: void f8(const __int128) {} + +void f8(const MaxSmall &) {} +// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'const MaxSmall' (aka 'const array') is a small, trivially-copyable type: pass by value instead +// CHECK-FIXES: void f8(const MaxSmall) {} + +template +void f8(T, const __int128 &) {} +// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'const __int128' is a small, trivially-copyable type: pass by value instead +// CHECK-FIXES: void f8(T, const __int128) {} + +template +void f8(const MaxSmall &, T) {} +// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'const MaxSmall' (aka 'const array') is a small, trivially-copyable type: pass by value instead +// CHECK-FIXES: void f8(const MaxSmall, T) {} + +struct S8 { + void f128(const __int128 &) {} + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 'const __int128' is a small, trivially-copyable type: pass by value instead + // CHECK-FIXES: void f128(const __int128) {} + + void fMaxSmall(const MaxSmall &) {} + // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: 'const MaxSmall' (aka 'const array') is a small, trivially-copyable type: pass by value instead + // CHECK-FIXES: void fMaxSmall(const MaxSmall) {} + + template + void template128(T, const __int128 &) {} + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'const __int128' is a small, trivially-copyable type: pass by value instead + // CHECK-FIXES: void template128(T, const __int128) {} + + template + void templateMaxSmall(const MaxSmall &, T) {} + // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: 'const MaxSmall' (aka 'const array') is a small, trivially-copyable type: pass by value instead + // CHECK-FIXES: void templateMaxSmall(const MaxSmall, T) {} +}; + +//////////////////////////////////////////////////////////////////////////////// +// Warn when passing a "large" or non-trivially-copyable parameter by value (and +// it's const-qualified). +//////////////////////////////////////////////////////////////////////////////// +void f9(const MinLarge x) {} +// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'const MinLarge' (aka 'const array') is a large type: pass by reference-to-const instead +// CHECK-FIXES: void f9(const MinLarge &x) {} + +void f9(const NotTriviallyCopyable x) {} +// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: 'const NotTriviallyCopyable' is not trivially-copyable: pass by reference-to-const instead +// CHECK-FIXES: void f9(const NotTriviallyCopyable &x) {} + +void f9(const array x) {} +// CHECK-MESSAGES: :[[@LINE-1]]:46: warning: 'const array' is not trivially-copyable: pass by reference-to-const instead +// CHECK-FIXES: void f9(const array &x) {} + +template +void f9(T, const MinLarge x) {} +// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 'const MinLarge' (aka 'const array') is a large type: pass by reference-to-const instead +// CHECK-FIXES: void f9(T, const MinLarge &x) {} + +template +void f9(const NotTriviallyCopyable x, T) {} +// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: 'const NotTriviallyCopyable' is not trivially-copyable: pass by reference-to-const instead +// CHECK-FIXES: void f9(const NotTriviallyCopyable &x, T) {} + +template +void f9(const array x, T) {} +// CHECK-MESSAGES: :[[@LINE-1]]:46: warning: 'const array' is not trivially-copyable: pass by reference-to-const instead +// CHECK-FIXES: void f9(const array &x, T) {} + +struct S9 { + void fMinLarge(const MinLarge x) {} + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: 'const MinLarge' (aka 'const array') is a large type: pass by reference-to-const instead + // CHECK-FIXES: void fMinLarge(const MinLarge &x) {} + + void fNTC(const NotTriviallyCopyable x) {} + // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: 'const NotTriviallyCopyable' is not trivially-copyable: pass by reference-to-const instead + // CHECK-FIXES: void fNTC(const NotTriviallyCopyable &x) {} + + void fTNTC(const array x) {} + // CHECK-MESSAGES: :[[@LINE-1]]:51: warning: 'const array' is not trivially-copyable: pass by reference-to-const instead + // CHECK-FIXES: void fTNTC(const array &x) {} + + template + void templateMinLarge(T, const MinLarge x) {} + // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: 'const MinLarge' (aka 'const array') is a large type: pass by reference-to-const instead + // CHECK-FIXES: void templateMinLarge(T, const MinLarge &x) {} + + template + void templateNTC(const NotTriviallyCopyable x, T) {} + // CHECK-MESSAGES: :[[@LINE-1]]:47: warning: 'const NotTriviallyCopyable' is not trivially-copyable: pass by reference-to-const instead + // CHECK-FIXES: void templateNTC(const NotTriviallyCopyable &x, T) {} + + template + void templateTNTC(const array x, T) {} + // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: 'const array' is not trivially-copyable: pass by reference-to-const instead + // CHECK-FIXES: void templateTNTC(const array &x, T) {} +}; + +//////////////////////////////////////////////////////////////////////////////// +// Default non-trivially copyable exemptions: no warnings expected +//////////////////////////////////////////////////////////////////////////////// + +namespace boost { +template +struct shared_ptr { + shared_ptr(const shared_ptr &); +}; + +void can_pass_by_value(const shared_ptr) {} +void can_pass_by_value(const shared_ptr>) {} + +void can_pass_by_ref_to_const(const shared_ptr &) {} +void can_pass_by_ref_to_const(const shared_ptr> &) {} +} // namespace boost + +namespace std { +template +struct shared_ptr { + shared_ptr(const shared_ptr &); +}; + +void can_pass_by_value(const shared_ptr) {} +void can_pass_by_value(const shared_ptr>) {} + +void can_pass_by_ref_to_const(const shared_ptr &) {} +void can_pass_by_ref_to_const(const shared_ptr> &) {} +} // namespace std diff --git a/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn b/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn --- a/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn +++ b/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn @@ -12,6 +12,7 @@ "//llvm/lib/Support", ] sources = [ + "ConstParameterValueOrRef.cpp", "FasterStringFindCheck.cpp", "ForRangeCopyCheck.cpp", "ImplicitConversionInLoopCheck.cpp",