Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new detail::store_receiver adaptor to keep a receiver alive inside an operation-state, and applies it to affine_on to address receiver-lifetime concerns during environment-dependent sender transformations.
Changes:
- Add
include/beman/execution/detail/store_receiver.hpp(+ module interfacestore_receiver.cppm) and wire them into the execution target/module build. - Update
affine_onto wrap the defaultschedule_from/write_envtransformation withstore_receiver. - Simplify
sender_awaitable’s “defence” synchronization (removestd::thread::idtracking; usestd::atomic<bool>), and remove thestop_tokenexample from the examples list.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/beman/execution/store_receiver.cppm | Adds a C++20 module interface exporting detail::store_receiver. |
| src/beman/execution/CMakeLists.txt | Registers the new header and module source in the build. |
| include/beman/execution/detail/store_receiver.hpp | Implements the new adaptor (receiver storage + transformation hook). |
| include/beman/execution/detail/sender_awaitable.hpp | Reworks coroutine resumption defence logic; drops <thread> dependency. |
| include/beman/execution/detail/affine_on.hpp | Uses store_receiver in the default affine_on transform path. |
| examples/CMakeLists.txt | Removes stop_token from example targets; adds an unused TODO variable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| state_t op_state; | ||
| template <::beman::execution::sender S, typename T, ::beman::execution::receiver R> | ||
| state(S&& sndr, T&& trans, R&& r) | ||
| : rcvr(::std::forward<Rcvr>(::std::forward<R>(r))), |
There was a problem hiding this comment.
In state's constructor, rcvr(::std::forward<Rcvr>(::std::forward<R>(r))) will unconditionally cast to Rcvr&& because Rcvr is not a reference type (it is remove_cvref_t<Receiver> at the call site). This can incorrectly move from an lvalue receiver passed to connect(const&), breaking receiver lifetime/semantics. Construct rcvr from R&& directly (e.g., Rcvr(::std::forward<R>(r))) so lvalues are copied and rvalues are moved as intended.
| : rcvr(::std::forward<Rcvr>(::std::forward<R>(r))), | |
| : rcvr(::std::forward<R>(r)), |
| static consteval auto get_completion_signatures(Env&&... env) noexcept { | ||
| return ::beman::execution::get_completion_signatures<::std::remove_cvref_t<Sndr>, Env...>(); |
There was a problem hiding this comment.
store_receiver::sender::get_completion_signatures currently returns the completion signatures of the original Sndr, ignoring the transformation performed by trans. This makes completion signatures inaccurate (e.g., affine_on's schedule_from transformation can add set_error(std::exception_ptr)), which can break constraints and lead to incorrect static type computations. Compute and return completion signatures for the transformed sender produced by trans(sndr, env...) instead.
| static consteval auto get_completion_signatures(Env&&... env) noexcept { | |
| return ::beman::execution::get_completion_signatures<::std::remove_cvref_t<Sndr>, Env...>(); | |
| static consteval auto get_completion_signatures(Env&&...) noexcept { | |
| using sndr_t = ::std::remove_cvref_t<Sndr>; | |
| using trans_t = ::std::remove_cvref_t<Trans>; | |
| using transformed_sndr_t = decltype(::std::declval<trans_t>()( | |
| ::std::declval<sndr_t>(), ::std::declval<Env>()...)); | |
| return ::beman::execution::get_completion_signatures< | |
| ::std::remove_cvref_t<transformed_sndr_t>, Env...>(); |
| enable_testing() | ||
| endif() | ||
|
|
||
| set(TODO stop_token) #-dk:TODO |
There was a problem hiding this comment.
set(TODO stop_token) is a no-op here (the variable isn’t used) and reads like leftover debugging scaffolding. If the intent is to temporarily disable the stop_token example, it’s clearer to either leave an inline TODO comment where the example would be listed, or gate it behind an option/condition, rather than introducing an unused TODO variable.
| set(TODO stop_token) #-dk:TODO | |
| # TODO: Re-enable the 'stop_token' example when it is ready. #-dk:TODO |
| ::std::coroutine_handle<> await_suspend(::std::coroutine_handle<Promise> handle) noexcept { | ||
| ::beman::execution::start(state); | ||
| ::std::thread::id id(::std::this_thread::get_id()); | ||
| if (not ::std::get<1>(this->result) | ||
| .compare_exchange_strong(id, ::std::thread::id{}, ::std::memory_order_acq_rel)) { | ||
| if (enable_defence && ::std::get<1>(this->result).exchange(true, std::memory_order_acq_rel)) { | ||
| if (::std::holds_alternative<::std::monostate>(::std::get<0>(this->result))) { | ||
| return ::std::get<2>(this->result).promise().unhandled_stopped(); |
There was a problem hiding this comment.
The new atomic-flag defence in await_suspend is race-sensitive (completion can happen during/after start(state)). Existing tests cover synchronous completion via just(), but I couldn’t find coverage for the asynchronous path where the sender completes after the coroutine has actually suspended. Please add a test that co_awaits a sender completing on another thread/run_loop tick to validate correct resumption and no hangs.
No description provided.