[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
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
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH 1/9] [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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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
Date: Wed, 21 May 2025 09:19:56 -0700
Subject: [PATCH 2/9] 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
+++
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
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
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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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
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
+++
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -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 `IsAllowedInCoroutines` with the default value `false` to carlosgalvezp wrote: Nit: typically we use the form `IgnoreX` for these types of things. I would then suggest naming the option `IgnoreCoroutines` and set it to `true` as default, i.e. revert the logic. https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
https://github.com/HerrCai0907 approved this pull request. https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -63,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(
+ unless(coroutineBodyStmt(,
vbvictor wrote:
For formatting par,t my initial suggestion had bad formatting so committing it
from github would give CI failure
https://github.com/llvm/llvm-project/pull/140912
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -63,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(
+ unless(coroutineBodyStmt(,
vbvictor wrote:
Note that I wrapped `unless(coroutineBodyStmt())` in a `stmt()` so the
resulting matcher would be `stmt(unless(coroutineBodyStmt()))`. Full line would
be like this:
```cpp
hasBody(IsAllowedInCoroutines
? stmt()
: stmt(unless(coroutineBodyStmt(
```
https://github.com/llvm/llvm-project/pull/140912
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
https://github.com/dmpolukhin edited https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -63,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(
+ unless(coroutineBodyStmt(,
dmpolukhin wrote:
Unfortunately this static_cast cannot be avoided because the second and third
arguments of the ternary operator have different types so it gives compiler
error and I was not able to find the way to avoid it without ugly conversion
(it can be C-style but I don't think it will make the code more readable). It
was the reason why I used check inside matcher callback in the previous version.
https://github.com/llvm/llvm-project/pull/140912
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -0,0 +1,65 @@ +// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- -fix-errors dmpolukhin wrote: I prefer to have 3 command to also test the default value for the flag. https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -0,0 +1,65 @@ +// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- -fix-errors vbvictor wrote: Do we need to have all 3 run commands? I suppose only 2 are needed, for `Coroutines = true` and `Coroutines = false` https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
https://github.com/vbvictor approved this pull request. LGTM, with minor suggestions https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
dmpolukhin wrote: @vbvictor please take another look, all comments resolved. https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
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
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH 1/6] [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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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
Date: Wed, 21 May 2025 09:19:56 -0700
Subject: [PATCH 2/6] 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
+++
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
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
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH 1/5] [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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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
Date: Wed, 21 May 2025 09:19:56 -0700
Subject: [PATCH 2/5] 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
+++
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -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 `IsAllowedInCoroutines` with the default value ``false`` to dmpolukhin wrote: Done https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -73,6 +74,10 @@ void
UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result)
{
const auto *Param = Result.Nodes.getNodeAs("param");
const auto *Function = Result.Nodes.getNodeAs("functionDecl");
+ if (!IsAllowedInCoroutines) {
+if (auto Body = Function->getBody(); Body && isa(Body))
+ return;
+ }
dmpolukhin wrote:
Done
https://github.com/llvm/llvm-project/pull/140912
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -74,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. dmpolukhin wrote: Done https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -0,0 +1,58 @@
+// 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 {
+
+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 {}; }
+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) {
dmpolukhin wrote:
Done
https://github.com/llvm/llvm-project/pull/140912
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -0,0 +1,58 @@
+// 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 -- \
dmpolukhin wrote:
`not` was the easiest way of tests with
`performance-unnecessary-value-param.IsAllowedInCoroutines: true` and it is
used see
[example](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp#L1).
But it is no longer relevant I updated test avoid it.
https://github.com/llvm/llvm-project/pull/140912
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
dmpolukhin wrote: @vbvictor and @HerrCai0907 @carlosgalvezp @PiotrZSL please take another look. https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
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
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH 1/4] [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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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
Date: Wed, 21 May 2025 09:19:56 -0700
Subject: [PATCH 2/4] 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
+++
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
vbvictor wrote: I think the default behavior of the option should be to ignore all coroutines - that will be to make ordinary users safe. https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
dmpolukhin wrote: > I have a mixed feeling about whether we should always exclude all coroutines > by default or not. When a coroutine has only one `co-operator` and nothing > else should be safe to use a reference parameter? Am I wrong? No, it may not be safe even in case of single co_return because of initial coroutine suspend point *before* entering function body. So if the parameter passed by reference is used inside coroutine it is not safe [see more here](https://en.cppreference.com/w/cpp/language/coroutines.html#:~:text=thus,%20may%20become%20dangling), except for the cases when the call is under co_await i.e. it is synchronous call. > So I think to be "on a safe side" I'd suggest a new option "IngoreCoroutines" > that will be `true` by default. If A user 100% knows that all references will > outlive the coroutine he could set it to `false`. In the option description > we could describe why it is not good to disable it as per cppcore-guidlines. > Generally speaking, I think it's not _very_ good to implement > _recommendations_ of cppcore-guidlines as a must in non-cppcore checks, they > should be under a flag. > > I'd be happy to accept your changes with this option, without - I'd want a > second opinion from a maintainer. Let's wait a bit more for maintainer, if no response, I'll add an option and this this case we will have a question what should be the default value for the option. https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
dmpolukhin wrote: @HerrCai0907 @carlosgalvezp @PiotrZSL one more friendly ping. Please take a look, the change is benign and trivial one liner + test. I think it will really improve this check and avoid long hours for developers to figure out dangling references. https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
vbvictor wrote: I have a mixed feeling about whether we should always exclude all coroutines by default or not. When a coroutine has only one `co-operator` and nothing else should be safe to use a reference parameter? Am I wrong? So I think to be "on a safe side" I'd suggest a new option "IngoreCoroutines" that will be `true` by default. If A user 100% knows that all references will outlive the coroutine he could set it to `false` and check performance issues:). In the option description, we could describe why it is not good to disable it. Generally speaking, maybe It's not _very_ good to implement _recommendations_ of cppcore-guidlines as a must in non-cppcore checks. I'd be happy to land your changes with this option, without - I'd want a second opinion from a maintainer. https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
dmpolukhin wrote: @HerrCai0907 @carlosgalvezp @PiotrZSL friendly ping, please take a look! https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
https://github.com/dmpolukhin edited https://github.com/llvm/llvm-project/pull/140912 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/140912
>From e3ce9bb18f165649b00db14b2282649315b28883 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH 1/3] [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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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 470bf837a1a958c75f4661c0c13dc7e48513d12d Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Wed, 21 May 2025 09:19:56 -0700
Subject: [PATCH 2/3] Add documentation and simplify test
---
clang-tools-extra/docs/ReleaseNotes.rst | 4 +-
.../performance/unnecessary-value-param.rst | 4 ++
.../unnecessary-value-param-coroutine.cpp | 63 ++-
3 files changed, 25 insertions(+), 46 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst
b/clang-tools-extra/docs/ReleaseNotes.rst
index 761c1d3a80359..9522b5ac8d6ca 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy -std=c++20-or-later %s
performance-unnecessary-value-param %t -- -fix-errors
+
+namespace std {
+
+template struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__builtin_coro_promise(ptr, alignof(Promise), false));
dmpolukhin wrote:
Yep, it is better example than used from
clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
https://github.com/llvm/llvm-project/pull/140912
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/140912
>From e3ce9bb18f165649b00db14b2282649315b28883 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH 1/2] [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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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 470bf837a1a958c75f4661c0c13dc7e48513d12d Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Wed, 21 May 2025 09:19:56 -0700
Subject: [PATCH 2/2] Add documentation and simplify test
---
clang-tools-extra/docs/ReleaseNotes.rst | 4 +-
.../performance/unnecessary-value-param.rst | 4 ++
.../unnecessary-value-param-coroutine.cpp | 63 ++-
3 files changed, 25 insertions(+), 46 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst
b/clang-tools-extra/docs/ReleaseNotes.rst
index 761c1d3a80359..9522b5ac8d6ca 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
llvmbot wrote:
@llvm/pr-subscribers-clang-tools-extra
Author: Dmitry Polukhin (dmpolukhin)
Changes
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
---
Full diff: https://github.com/llvm/llvm-project/pull/140912.diff
2 Files Affected:
- (modified)
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (+1)
- (added)
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
(+81)
``diff
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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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;
+}
``
https://github.com/llvm/llvm-project/pull/140912
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
https://github.com/dmpolukhin created
https://github.com/llvm/llvm-project/pull/140912
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
>From e3ce9bb18f165649b00db14b2282649315b28883 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Wed, 21 May 2025 07:54:02 -0700
Subject: [PATCH] [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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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;
+}
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Avoid in coroutines (PR #140912)
llvmbot wrote:
@llvm/pr-subscribers-clang-tidy
Author: Dmitry Polukhin (dmpolukhin)
Changes
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
---
Full diff: https://github.com/llvm/llvm-project/pull/140912.diff
2 Files Affected:
- (modified)
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (+1)
- (added)
clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
(+81)
``diff
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 0..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 struct coroutine_traits {
+ using promise_type = typename R::promise_type;
+};
+
+template struct coroutine_handle;
+
+template <> struct coroutine_handle {
+ 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 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(
+__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;
+}
``
https://github.com/llvm/llvm-project/pull/140912
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
