[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
Dmitry Polukhin via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 16 03:22:04 PDT 2025
https://github.com/dmpolukhin updated https://github.com/llvm/llvm-project/pull/140912
>From f277796a98f81bc3f0c6949adff3ad43eda51d5f Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH 1/8] [clang-tidy][performance-unnecessary-value-param] Avoid
in coroutines
Summary:
Replacing by-value parameters with passing by-reference is not safe for coroutines
because the caller may be executed in parallel with the callee, which increases
the chances of resulting in dangling references and hard-to-find crashes.
Test Plan: check-clang-tools
---
.../UnnecessaryValueParamCheck.cpp | 1 +
.../unnecessary-value-param-coroutine.cpp | 81 +++++++++++++++++++
2 files changed, 82 insertions(+)
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index a877f9a7ee912..52d4c70a83f65 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -65,6 +65,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
TK_AsIs,
functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()),
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
+ unless(hasBody(coroutineBodyStmt())),
has(typeLoc(forEach(ExpensiveValueParamDecl))),
decl().bind("functionDecl"))),
this);
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..f2fb477143578
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- -fix-errors
+
+namespace std {
+
+template <typename R, typename...> struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template <typename Promise = void> struct coroutine_handle;
+
+template <> struct coroutine_handle<void> {
+ static coroutine_handle from_address(void *addr) noexcept {
+ coroutine_handle me;
+ me.ptr = addr;
+ return me;
+ }
+ void operator()() { resume(); }
+ void *address() const noexcept { return ptr; }
+ void resume() const { }
+ void destroy() const { }
+ bool done() const { return true; }
+ coroutine_handle &operator=(decltype(nullptr)) {
+ ptr = nullptr;
+ return *this;
+ }
+ coroutine_handle(decltype(nullptr)) : ptr(nullptr) {}
+ coroutine_handle() : ptr(nullptr) {}
+ // void reset() { ptr = nullptr; } // add to P0057?
+ explicit operator bool() const { return ptr; }
+
+protected:
+ void *ptr;
+};
+
+template <typename Promise> struct coroutine_handle : coroutine_handle<> {
+ using coroutine_handle<>::operator=;
+
+ static coroutine_handle from_address(void *addr) noexcept {
+ coroutine_handle me;
+ me.ptr = addr;
+ return me;
+ }
+
+ Promise &promise() const {
+ return *reinterpret_cast<Promise *>(
+ __builtin_coro_promise(ptr, alignof(Promise), false));
+ }
+ static coroutine_handle from_promise(Promise &promise) {
+ coroutine_handle p;
+ p.ptr = __builtin_coro_promise(&promise, alignof(Promise), true);
+ return p;
+ }
+};
+
+struct suspend_always {
+ bool await_ready() noexcept { return false; }
+ void await_suspend(std::coroutine_handle<>) noexcept {}
+ void await_resume() noexcept {}
+};
+
+} // namespace std
+
+struct ReturnObject {
+ struct promise_type {
+ ReturnObject get_return_object() { 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 evaluateModels(const A timeMachineId) {
+// No change for non-coroutine function expected because it is not safe.
+// CHECK-FIXES: ReturnObject evaluateModels(const A timeMachineId) {
+ co_return;
+}
>From 011624d536cd5641be54260c3eca2980d4094b45 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Wed, 21 May 2025 09:19:56 -0700
Subject: [PATCH 2/8] Add documentation and simplify test
---
clang-tools-extra/docs/ReleaseNotes.rst | 2 +
.../performance/unnecessary-value-param.rst | 4 ++
.../unnecessary-value-param-coroutine.cpp | 63 ++++++-------------
3 files changed, 24 insertions(+), 45 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 19ccd1790e757..f1a8819e21788 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -265,6 +265,8 @@ Changes in existing checks
<clang-tidy/checks/performance/unnecessary-value-param>` 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.
+ Also suppressed this check for coroutine functions because it may not be safe
+ and suggested fixes may result in hard-to-find bugs and crashes.
- Improved :doc:`readability-convert-member-functions-to-static
<clang-tidy/checks/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..665e0a6360723 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
@@ -18,6 +18,10 @@ the following heuristic is employed:
on it, or it is used as const reference or value argument in constructors or
function calls.
+Note: This check does not suggest passing parameters by reference in coroutines
+because, after a coroutine suspend point, references could be dangling and no
+longer valid, so suggested changes may result in hard-to-find bugs and crashes.
+
Example:
.. code-block:: c++
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
index f2fb477143578..b6e9bd0a60d44 100644
--- 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
@@ -2,59 +2,32 @@
namespace std {
-template <typename R, typename...> struct coroutine_traits {
- using promise_type = typename R::promise_type;
+template <class Ret, typename... T> struct coroutine_traits {
+ using promise_type = typename Ret::promise_type;
};
-template <typename Promise = void> struct coroutine_handle;
-
-template <> struct coroutine_handle<void> {
- static coroutine_handle from_address(void *addr) noexcept {
- coroutine_handle me;
- me.ptr = addr;
- return me;
- }
- void operator()() { resume(); }
- void *address() const noexcept { return ptr; }
- void resume() const { }
- void destroy() const { }
- bool done() const { return true; }
- coroutine_handle &operator=(decltype(nullptr)) {
- ptr = nullptr;
- return *this;
- }
- coroutine_handle(decltype(nullptr)) : ptr(nullptr) {}
- coroutine_handle() : ptr(nullptr) {}
- // void reset() { ptr = nullptr; } // add to P0057?
- explicit operator bool() const { return ptr; }
-
-protected:
- void *ptr;
+template <class Promise = void> struct coroutine_handle {
+ static coroutine_handle from_address(void *) noexcept;
+ static coroutine_handle from_promise(Promise &promise);
+ constexpr void *address() const noexcept;
};
-template <typename Promise> struct coroutine_handle : coroutine_handle<> {
- using coroutine_handle<>::operator=;
-
- static coroutine_handle from_address(void *addr) noexcept {
- coroutine_handle me;
- me.ptr = addr;
- return me;
- }
-
- Promise &promise() const {
- return *reinterpret_cast<Promise *>(
- __builtin_coro_promise(ptr, alignof(Promise), false));
- }
- static coroutine_handle from_promise(Promise &promise) {
- coroutine_handle p;
- p.ptr = __builtin_coro_promise(&promise, alignof(Promise), true);
- return p;
- }
+template <> struct coroutine_handle<void> {
+ template <class PromiseType>
+ coroutine_handle(coroutine_handle<PromiseType>) noexcept;
+ static coroutine_handle from_address(void *);
+ constexpr void *address() const noexcept;
};
struct suspend_always {
bool await_ready() noexcept { return false; }
- void await_suspend(std::coroutine_handle<>) noexcept {}
+ 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 {}
};
>From 6f53984f854310522d37b8d0315e33443bbbc7eb Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Wed, 21 May 2025 09:54:34 -0700
Subject: [PATCH 3/8] Move note to more appropiate place
---
.../checks/performance/unnecessary-value-param.rst | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
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 665e0a6360723..fce3bec00504e 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
@@ -18,10 +18,6 @@ the following heuristic is employed:
on it, or it is used as const reference or value argument in constructors or
function calls.
-Note: This check does not suggest passing parameters by reference in coroutines
-because, after a coroutine suspend point, references could be dangling and no
-longer valid, so suggested changes may result in hard-to-find bugs and crashes.
-
Example:
.. code-block:: c++
@@ -60,7 +56,11 @@ 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.
+
+Note: This check does not suggest passing parameters by reference in coroutines
+because, after a coroutine suspend point, references could be dangling and no
+longer valid, so suggested changes may result in hard-to-find bugs and crashes.
Options
-------
>From 47640912484b2e32cbe9fa51b83c18552e7cb6dd Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Mon, 9 Jun 2025 06:19:43 -0700
Subject: [PATCH 4/8] Added option IsAllowedInCoroutines
---
.../performance/UnnecessaryValueParamCheck.cpp | 9 +++++++--
.../performance/UnnecessaryValueParamCheck.h | 1 +
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++--
.../checks/performance/unnecessary-value-param.rst | 10 ++++++----
.../performance/unnecessary-value-param-coroutine.cpp | 4 ++++
5 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index 52d4c70a83f65..b3e5910e113b5 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", ""))),
+ IsAllowedInCoroutines(Options.get("IsAllowedInCoroutines", false)) {}
void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
const auto ExpensiveValueParamDecl = parmVarDecl(
@@ -65,7 +66,6 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
TK_AsIs,
functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()),
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
- unless(hasBody(coroutineBodyStmt())),
has(typeLoc(forEach(ExpensiveValueParamDecl))),
decl().bind("functionDecl"))),
this);
@@ -74,6 +74,10 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
+ if (!IsAllowedInCoroutines) {
+ if (auto Body = Function->getBody(); Body && isa<CoroutineBodyStmt>(Body))
+ return;
+ }
TraversalKindScope RAII(*Result.Context, TK_AsIs);
@@ -124,6 +128,7 @@ void UnnecessaryValueParamCheck::storeOptions(
Options.store(Opts, "IncludeStyle", Inserter.getStyle());
Options.store(Opts, "AllowedTypes",
utils::options::serializeStringList(AllowedTypes));
+ Options.store(Opts, "IsAllowedInCoroutines", IsAllowedInCoroutines);
}
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..8101bf6e8306d 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<StringRef> AllowedTypes;
+ bool IsAllowedInCoroutines;
};
} // namespace clang::tidy::performance
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f1a8819e21788..761a466b43e2a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -265,8 +265,8 @@ Changes in existing checks
<clang-tidy/checks/performance/unnecessary-value-param>` 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.
- Also suppressed this check for coroutine functions because it may not be safe
- and suggested fixes may result in hard-to-find bugs and crashes.
+ Added an option `IsAllowedInCoroutines` with the default value ``false`` to
+ suppress this check for coroutines where passing by reference may be unsafe.
- Improved :doc:`readability-convert-member-functions-to-static
<clang-tidy/checks/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 fce3bec00504e..459042784345d 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
@@ -58,10 +58,6 @@ 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 function signatures.
-Note: This check does not suggest passing parameters by reference in coroutines
-because, after a coroutine suspend point, references could be dangling and no
-longer valid, so suggested changes may result in hard-to-find bugs and crashes.
-
Options
-------
@@ -78,3 +74,9 @@ 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:: IsAllowedInCoroutines
+
+ A boolean specifying whether the check should suggest passing parameters by
+ reference in coroutines. The default is `false`. Passing parameters by reference
+ in coroutines may not be safe.
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
index b6e9bd0a60d44..52e86f8d32af4 100644
--- 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
@@ -1,4 +1,8 @@
// 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.IsAllowedInCoroutines: false}}' -fix-errors
+// RUN: not %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- \
+// RUN: -config='{CheckOptions: {performance-unnecessary-value-param.IsAllowedInCoroutines: true}}' -fix-errors
namespace std {
>From 082f1c848e0830f3b8b0057edb24b40511948117 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Thu, 12 Jun 2025 04:31:06 -0700
Subject: [PATCH 5/8] Accepted suggested changes
---
.../performance/UnnecessaryValueParamCheck.cpp | 11 ++++++-----
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
.../performance/unnecessary-value-param.rst | 5 +++--
.../unnecessary-value-param-coroutine.cpp | 15 +++++++++++----
4 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index b3e5910e113b5..65c3bc63eaec4 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -54,6 +54,7 @@ UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
IsAllowedInCoroutines(Options.get("IsAllowedInCoroutines", false)) {}
void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
+ using StmtMatcher = ast_matchers::internal::BindableMatcher<Stmt>;
const auto ExpensiveValueParamDecl = parmVarDecl(
hasType(qualType(
hasCanonicalType(matchers::isExpensiveToCopy()),
@@ -64,7 +65,11 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
traverse(
TK_AsIs,
- functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()),
+ functionDecl(hasBody(IsAllowedInCoroutines
+ ? stmt()
+ : static_cast<StmtMatcher>(
+ unless(coroutineBodyStmt()))),
+ isDefinition(), unless(isImplicit()),
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
has(typeLoc(forEach(ExpensiveValueParamDecl))),
decl().bind("functionDecl"))),
@@ -74,10 +79,6 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
- if (!IsAllowedInCoroutines) {
- if (auto Body = Function->getBody(); Body && isa<CoroutineBodyStmt>(Body))
- return;
- }
TraversalKindScope RAII(*Result.Context, TK_AsIs);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 761a466b43e2a..f677fcde321c8 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -265,7 +265,7 @@ Changes in existing checks
<clang-tidy/checks/performance/unnecessary-value-param>` 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 `IsAllowedInCoroutines` with the default value ``false`` to
+ Added an option `IsAllowedInCoroutines` with the default value `false` to
suppress this check for coroutines where passing by reference may be unsafe.
- Improved :doc:`readability-convert-member-functions-to-static
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 459042784345d..e6091449e2e4d 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
@@ -78,5 +78,6 @@ Options
.. option:: IsAllowedInCoroutines
A boolean specifying whether the check should suggest passing parameters by
- reference in coroutines. The default is `false`. Passing parameters by reference
- in coroutines may not be safe.
+ 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 `false`.
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
index 52e86f8d32af4..7cceb2fc64d93 100644
--- 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
@@ -1,7 +1,7 @@
// 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.IsAllowedInCoroutines: false}}' -fix-errors
-// RUN: not %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- \
+// 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.IsAllowedInCoroutines: true}}' -fix-errors
namespace std {
@@ -40,6 +40,7 @@ struct suspend_never {
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() {}
@@ -51,8 +52,14 @@ struct A {
A(const A&);
};
-ReturnObject evaluateModels(const A timeMachineId) {
-// No change for non-coroutine function expected because it is not safe.
-// CHECK-FIXES: ReturnObject evaluateModels(const A timeMachineId) {
+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{};
+}
>From 2fc80172c8fe3425ba9efc8c20cb3cd467ceaa57 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Thu, 12 Jun 2025 05:11:42 -0700
Subject: [PATCH 6/8] Fix formatting
---
.../clang-tidy/checks/performance/unnecessary-value-param.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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 e6091449e2e4d..0f9c7536c0c91 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
@@ -79,5 +79,5 @@ Options
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 `false`.
+ not be safe, please see :doc:`cppcoreguidelines-avoid-reference-coroutine-parameters <../cppcoreguidelines/avoid-reference-coroutine-parameters>`
+ for more information. Default is `false`.
>From c5b7ddaa345a0ae5393dbe9e517e42bc8ef1dfd9 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Fri, 13 Jun 2025 06:40:55 -0700
Subject: [PATCH 7/8] Remove unnecessary static_cast.
---
.../clang-tidy/performance/UnnecessaryValueParamCheck.cpp | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index 65c3bc63eaec4..8cc0a63243c39 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -54,7 +54,6 @@ UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
IsAllowedInCoroutines(Options.get("IsAllowedInCoroutines", false)) {}
void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
- using StmtMatcher = ast_matchers::internal::BindableMatcher<Stmt>;
const auto ExpensiveValueParamDecl = parmVarDecl(
hasType(qualType(
hasCanonicalType(matchers::isExpensiveToCopy()),
@@ -67,8 +66,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
TK_AsIs,
functionDecl(hasBody(IsAllowedInCoroutines
? stmt()
- : static_cast<StmtMatcher>(
- unless(coroutineBodyStmt()))),
+ : stmt(unless(coroutineBodyStmt()))),
isDefinition(), unless(isImplicit()),
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
has(typeLoc(forEach(ExpensiveValueParamDecl))),
>From c0c5fdcac3bcf4c46d30cc4b15ebbfa481c68117 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Mon, 16 Jun 2025 03:20:46 -0700
Subject: [PATCH 8/8] s/IsAllowedInCoroutines/IgnoreCoroutines/ and invert
logic
---
.../performance/UnnecessaryValueParamCheck.cpp | 10 +++++-----
.../performance/UnnecessaryValueParamCheck.h | 2 +-
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
.../checks/performance/unnecessary-value-param.rst | 4 ++--
.../performance/unnecessary-value-param-coroutine.cpp | 4 ++--
5 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index 8cc0a63243c39..6771fdd0ce37b 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -51,7 +51,7 @@ UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
areDiagsSelfContained()),
AllowedTypes(
utils::options::parseStringList(Options.get("AllowedTypes", ""))),
- IsAllowedInCoroutines(Options.get("IsAllowedInCoroutines", false)) {}
+ IgnoreCoroutines(Options.get("IgnoreCoroutines", true)) {}
void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
const auto ExpensiveValueParamDecl = parmVarDecl(
@@ -64,9 +64,9 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
traverse(
TK_AsIs,
- functionDecl(hasBody(IsAllowedInCoroutines
- ? stmt()
- : stmt(unless(coroutineBodyStmt()))),
+ functionDecl(hasBody(IgnoreCoroutines
+ ? stmt(unless(coroutineBodyStmt()))
+ : stmt()),
isDefinition(), unless(isImplicit()),
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
has(typeLoc(forEach(ExpensiveValueParamDecl))),
@@ -127,7 +127,7 @@ void UnnecessaryValueParamCheck::storeOptions(
Options.store(Opts, "IncludeStyle", Inserter.getStyle());
Options.store(Opts, "AllowedTypes",
utils::options::serializeStringList(AllowedTypes));
- Options.store(Opts, "IsAllowedInCoroutines", IsAllowedInCoroutines);
+ 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 8101bf6e8306d..b52043416e769 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h
@@ -46,7 +46,7 @@ class UnnecessaryValueParamCheck : public ClangTidyCheck {
ExprMutationAnalyzer::Memoized MutationAnalyzerCache;
utils::IncludeInserter Inserter;
const std::vector<StringRef> AllowedTypes;
- bool IsAllowedInCoroutines;
+ bool IgnoreCoroutines;
};
} // namespace clang::tidy::performance
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f677fcde321c8..3c1ca2f929044 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -265,7 +265,7 @@ Changes in existing checks
<clang-tidy/checks/performance/unnecessary-value-param>` 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 `IsAllowedInCoroutines` with the default value `false` to
+ 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
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 0f9c7536c0c91..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
@@ -75,9 +75,9 @@ Options
matched against the qualified type name (i.e. ``namespace::Type``),
otherwise it is matched against only the type name (i.e. ``Type``).
-.. option:: IsAllowedInCoroutines
+.. 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 `false`.
+ 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
index 7cceb2fc64d93..0a84dc4676470 100644
--- 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
@@ -1,8 +1,8 @@
// 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.IsAllowedInCoroutines: false}}' -fix-errors
+// 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.IsAllowedInCoroutines: true}}' -fix-errors
+// RUN: -config='{CheckOptions: {performance-unnecessary-value-param.IgnoreCoroutines: false}}' -fix-errors
namespace std {
More information about the cfe-commits
mailing list