Hi Jason, Thanks for the review.
Jason Merrill <ja...@redhat.com> writes:
> On 7/23/24 7:41 PM, Arsen Arsenović wrote:
>> co_await expressions are nearly calls to Awaitable::await_resume, and,
>> as such, should inherit its nodiscard. A discarded co_await expression
>> should, hence, act as if its call to await_resume was discarded.
>> CO_AWAIT_EXPR trees do conveniently contain the expression for calling
>> await_resume in them, so we can discard it.
>> gcc/cp/ChangeLog:
>> PR c++/110171
>> * coroutines.cc (co_await_get_resume_call): New function.
>> Returns the await_resume expression of a given co_await.
>> * cp-tree.h (co_await_get_resume_call): New function.
>> * cvt.cc (convert_to_void): Handle CO_AWAIT_EXPRs and call
>> maybe_warn_nodiscard on their resume exprs.
>> gcc/testsuite/ChangeLog:
>> PR c++/110171
>> * g++.dg/coroutines/pr110171-1.C: New test.
>> * g++.dg/coroutines/pr110171.C: New test.
>> ---
>> This patch teaches convert_to_void how to discard 'through' a
>> CO_AWAIT_EXPR. CO_AWAIT_EXPR nodes (most of the time) already contain
>> their relevant await_resume() call embedded within them, so, when we
>> discard a CO_AWAIT_EXPR, we can also just discard the await_resume()
>> call embedded within it. This results in a [[nodiscard]] diagnostic
>> that the PR noted was missing.
>
> Again you have two different versions of the patch rationale?
The duplication is for no particular reason - I just have a habit of
always filling out the email body besides just the commit message. In
the future, I'll move most text into the commit message, and I'll merge
all discussion from both versions into the commit message and post them
for review before pushing (for this patch and the previous).
>> As with the previous patch, regression-tested on x86_64-pc-linux-gnu.
>> OK for trunk?
>> TIA.
>> gcc/cp/coroutines.cc | 13 ++++++++
>> gcc/cp/cp-tree.h | 3 ++
>> gcc/cp/cvt.cc | 8 +++++
>> gcc/testsuite/g++.dg/coroutines/pr110171-1.C | 34 ++++++++++++++++++++
>> gcc/testsuite/g++.dg/coroutines/pr110171.C | 32 ++++++++++++++++++
>> 5 files changed, 90 insertions(+)
>> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr110171-1.C
>> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr110171.C
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index fb8f24e6c61..05486c2fb19 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -596,6 +596,19 @@ coro_get_destroy_function (tree decl)
>> return NULL_TREE;
>> }
>> +/* Given a CO_AWAIT_EXPR AWAIT_EXPR, return its resume call. */
>> +
>> +tree*
>
> Why return tree* rather than tree?
So that we can save the result of convert_to_void on the resume
expression back into the CO_AWAIT_EXPR:
case CO_AWAIT_EXPR:
{
auto awr = co_await_get_resume_call (expr);
if (awr && *awr)
*awr = convert_to_void (*awr, implicit, complain);
break;
}
>> +co_await_get_resume_call (tree await_expr)
>> +{
>> + gcc_checking_assert (TREE_CODE (await_expr) == CO_AWAIT_EXPR);
>> + tree vec = TREE_OPERAND (await_expr, 3);
>> + if (!vec)
>> + return nullptr;
>> + return &TREE_VEC_ELT (vec, 2);
>> +}
>> +
>> +
>> /* These functions assumes that the caller has verified that the state for
>> the decl has been initialized, we try to minimize work here. */
>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>> index 856699de82f..c9ae8950bd1 100644
>> --- a/gcc/cp/cp-tree.h
>> +++ b/gcc/cp/cp-tree.h
>> @@ -8763,6 +8763,9 @@ extern tree coro_get_actor_function (tree);
>> extern tree coro_get_destroy_function (tree);
>> extern tree coro_get_ramp_function (tree);
>> +extern tree* co_await_get_resume_call (tree await_expr);
>> +
>> +
>> /* contracts.cc */
>> extern tree make_postcondition_variable (cp_expr);
>> extern tree make_postcondition_variable (cp_expr, tree);
>> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
>> index d95e01c118c..7b4bd8a9dc4 100644
>> --- a/gcc/cp/cvt.cc
>> +++ b/gcc/cp/cvt.cc
>> @@ -1502,6 +1502,14 @@ convert_to_void (tree expr, impl_conv_void implicit,
>> tsubst_flags_t complain)
>> maybe_warn_nodiscard (expr, implicit);
>> break;
>> + case CO_AWAIT_EXPR:
>> + {
>> + auto awr = co_await_get_resume_call (expr);
>> + if (awr && *awr)
>> + *awr = convert_to_void (*awr, implicit, complain);
>> + break;
>> + }
>> +
>> default:;
>> }
>> expr = resolve_nondeduced_context (expr, complain);
>> diff --git a/gcc/testsuite/g++.dg/coroutines/pr110171-1.C
>> b/gcc/testsuite/g++.dg/coroutines/pr110171-1.C
>> new file mode 100644
>> index 00000000000..d8aff582487
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/coroutines/pr110171-1.C
>> @@ -0,0 +1,34 @@
>> +// { dg-do compile }
>> +#include <coroutine>
>> +
>> +struct must_check_result
>> +{
>> + bool await_ready() { return false; }
>> + void await_suspend(std::coroutine_handle<>) {}
>> + [[nodiscard]] bool await_resume() { return {}; }
>> +};
>> +
>> +struct task {};
>> +
>> +namespace std
>> +{
>> + template<typename... Args>
>> + struct coroutine_traits<task, Args...>
>> + {
>> + struct promise_type
>> + {
>> + task get_return_object() { return {}; }
>> + suspend_always initial_suspend() noexcept { return {}; }
>> + suspend_always final_suspend() noexcept { return {}; }
>> + void return_void() {}
>> + void unhandled_exception() {}
>> + };
>> + };
>> +}
>> +
>> +task example(auto)
>> +{
>> + co_await must_check_result(); // { dg-warning "-Wunused-result" }
>> +}
>> +
>> +void f() { example(1); }
>> diff --git a/gcc/testsuite/g++.dg/coroutines/pr110171.C
>> b/gcc/testsuite/g++.dg/coroutines/pr110171.C
>> new file mode 100644
>> index 00000000000..4b82e23656c
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/coroutines/pr110171.C
>> @@ -0,0 +1,32 @@
>> +// { dg-do compile }
>> +#include <coroutine>
>> +
>> +struct must_check_result
>> +{
>> + bool await_ready() { return false; }
>> + void await_suspend(std::coroutine_handle<>) {}
>> + [[nodiscard]] bool await_resume() { return {}; }
>> +};
>> +
>> +struct task {};
>> +
>> +namespace std
>> +{
>> + template<typename... Args>
>> + struct coroutine_traits<task, Args...>
>> + {
>> + struct promise_type
>> + {
>> + task get_return_object() { return {}; }
>> + suspend_always initial_suspend() noexcept { return {}; }
>> + suspend_always final_suspend() noexcept { return {}; }
>> + void return_void() {}
>> + void unhandled_exception() {}
>> + };
>> + };
>> +}
>> +
>> +task example()
>> +{
>> + co_await must_check_result(); // { dg-warning "-Wunused-result" }
>> +}
--
Arsen Arsenović
signature.asc
Description: PGP signature
