diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp index a877f9a7ee912..d89c3a69fc841 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp @@ -50,7 +50,8 @@ UnnecessaryValueParamCheck::UnnecessaryValueParamCheck( utils::IncludeSorter::IS_LLVM), areDiagsSelfContained()), AllowedTypes( - utils::options::parseStringList(Options.get("AllowedTypes", ""))) {} + utils::options::parseStringList(Options.get("AllowedTypes", ""))), + IgnoreCoroutines(Options.get("IgnoreCoroutines", true)) {} void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) { const auto ExpensiveValueParamDecl = parmVarDecl( @@ -61,12 +62,14 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) { matchers::matchesAnyListedName(AllowedTypes))))))), decl().bind("param")); Finder->addMatcher( - traverse( - TK_AsIs, - functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()), - unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))), - has(typeLoc(forEach(ExpensiveValueParamDecl))), - decl().bind("functionDecl"))), + traverse(TK_AsIs, + functionDecl( + hasBody(IgnoreCoroutines ? stmt(unless(coroutineBodyStmt())) + : stmt()), + isDefinition(), unless(isImplicit()), + unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))), + has(typeLoc(forEach(ExpensiveValueParamDecl))), + decl().bind("functionDecl"))), this); } @@ -123,6 +126,7 @@ void UnnecessaryValueParamCheck::storeOptions( Options.store(Opts, "IncludeStyle", Inserter.getStyle()); Options.store(Opts, "AllowedTypes", utils::options::serializeStringList(AllowedTypes)); + Options.store(Opts, "IgnoreCoroutines", IgnoreCoroutines); } void UnnecessaryValueParamCheck::onEndOfTranslationUnit() { diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h index 8bfd814d16357..b52043416e769 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h @@ -46,6 +46,7 @@ class UnnecessaryValueParamCheck : public ClangTidyCheck { ExprMutationAnalyzer::Memoized MutationAnalyzerCache; utils::IncludeInserter Inserter; const std::vector AllowedTypes; + bool IgnoreCoroutines; }; } // namespace clang::tidy::performance diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 19ccd1790e757..3c1ca2f929044 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -265,6 +265,8 @@ Changes in existing checks ` check performance by tolerating fix-it breaking compilation when functions is used as pointers to avoid matching usage of functions within the current compilation unit. + Added an option `IgnoreCoroutines` with the default value `true` to + suppress this check for coroutines where passing by reference may be unsafe. - Improved :doc:`readability-convert-member-functions-to-static ` check by diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst index dc86530b95f13..cd25d7d94d99b 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-value-param.rst @@ -56,7 +56,7 @@ Will become: Because the fix-it needs to change the signature of the function, it may break builds if the function is used in multiple translation units or some codes -depends on funcion signatures. +depends on function signatures. Options ------- @@ -74,3 +74,10 @@ Options default is empty. If a name in the list contains the sequence `::`, it is matched against the qualified type name (i.e. ``namespace::Type``), otherwise it is matched against only the type name (i.e. ``Type``). + +.. option:: IgnoreCoroutines + + A boolean specifying whether the check should suggest passing parameters by + reference in coroutines. Passing parameters by reference in coroutines may + not be safe, please see :doc:`cppcoreguidelines-avoid-reference-coroutine-parameters <../cppcoreguidelines/avoid-reference-coroutine-parameters>` + for more information. Default is `true`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp new file mode 100644 index 0000000000000..0a84dc4676470 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp @@ -0,0 +1,65 @@ +// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- -fix-errors +// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- \ +// RUN: -config='{CheckOptions: {performance-unnecessary-value-param.IgnoreCoroutines: true}}' -fix-errors +// RUN: %check_clang_tidy -check-suffix=ALLOWED -std=c++20-or-later %s performance-unnecessary-value-param %t -- \ +// RUN: -config='{CheckOptions: {performance-unnecessary-value-param.IgnoreCoroutines: false}}' -fix-errors + +namespace std { + +template struct coroutine_traits { + using promise_type = typename Ret::promise_type; +}; + +template struct coroutine_handle { + static coroutine_handle from_address(void *) noexcept; + static coroutine_handle from_promise(Promise &promise); + constexpr void *address() const noexcept; +}; + +template <> struct coroutine_handle { + template + coroutine_handle(coroutine_handle) noexcept; + static coroutine_handle from_address(void *); + constexpr void *address() const noexcept; +}; + +struct suspend_always { + bool await_ready() noexcept { return false; } + void await_suspend(coroutine_handle<>) noexcept {} + void await_resume() noexcept {} +}; + +struct suspend_never { + bool await_ready() noexcept { return true; } + void await_suspend(coroutine_handle<>) noexcept {} + void await_resume() noexcept {} +}; + +} // namespace std + +struct ReturnObject { + struct promise_type { + ReturnObject get_return_object() { return {}; } + ReturnObject return_void() { return {}; } + std::suspend_always initial_suspend() { return {}; } + std::suspend_always final_suspend() noexcept { return {}; } + void unhandled_exception() {} + std::suspend_always yield_value(int value) { return {}; } + }; +}; + +struct A { + A(const A&); +}; + +ReturnObject foo_coroutine(const A a) { +// CHECK-MESSAGES-ALLOWED: [[@LINE-1]]:36: warning: the const qualified parameter 'a' +// CHECK-FIXES: ReturnObject foo_coroutine(const A a) { + co_return; +} + +ReturnObject foo_not_coroutine(const A a) { +// CHECK-MESSAGES: [[@LINE-1]]:40: warning: the const qualified parameter 'a' +// CHECK-MESSAGES-ALLOWED: [[@LINE-2]]:40: warning: the const qualified parameter 'a' + return ReturnObject{}; +}