Repository navigation
exec(fork_join): coalesce empty and unary calls - #2302
romintomasetti wants to merge 2 commits into
Conversation
|
@RobertLeahy @ericniebler Could you please have a look? Thanks! |
|
|
||
| /// Unary closure. | ||
| template <STDEXEC::sender Sndr, class Closure> requires (!STDEXEC::sender<Closure>) | ||
| constexpr auto operator()(Sndr&& sndr, Closure&& clsr) const noexcept(STDEXEC::__nothrow_decay_copyable<Closure>) |
There was a problem hiding this comment.
I don't think this noexcept is correct. Just because Closure can be decay-copied doesn't mean binding it together with something of type Sndr can't.
There was a problem hiding this comment.
Indeed! How would you do it, apart from
noexcept(noexcept(static_cast<Sndr&&>(sndr) | static_cast<Closure&&>(clsr)))?
There was a problem hiding this comment.
Use inspiration from include/stdexec/__detail/__sender_adaptor_closure.hpp?
There was a problem hiding this comment.
Done. What do you think?
1a93249 to
376da8c
Compare
376da8c to
e910941
Compare
Signed-off-by: romintomasetti <romin.tomasetti@gmail.com>
e910941 to
91e17da
Compare
| /// Multiple closures. | ||
| auto multiple = STDEXEC::just() | exec::fork_join(then, then); | ||
| STATIC_REQUIRE(exec::sender_for<decltype(multiple), exec::fork_join_t>); | ||
| STATIC_REQUIRE(!noexcept(exec::fork_join(STDEXEC::just(), then, then))); |
There was a problem hiding this comment.
We could also omit this line. Because it seems the reason is that the function has no noexcept clause, rather than that one operations may throw.
There was a problem hiding this comment.
Yeah, but at least it's tested.
There was a problem hiding this comment.
I would expect constructing such an exec::fork_join sender not to throw, which noexcept is missing that prevents that?
There was a problem hiding this comment.
There are 2 functions in the fork_join_t struct I haven't touched yet.
There was a problem hiding this comment.
Why would we add tests locking in broken/incorrect/inaccurate behavior?
There was a problem hiding this comment.
My two cents would be to add the noexcept clause and flip the static_assert here.
We could write something like (perhaps there's a way to make it less verbose with other concepts from __concepts.hpp):
noexcept(STDEXEC::__nothrow_decay_copyable<Closures..., Sndr>
&& (std::is_nothrow_move_constructible_v<STDEXEC::__decay_t<Closures>> && ...)
&& std::is_nothrow_move_constructible_v<STDEXEC::__decay_t<Sndr>>)
The last two checks would be needed for the moves here:
stdexec/include/stdexec/__detail/__basic_sender.hpp
Lines 460 to 470 in f4c123f
Similar to NVIDIA#2124. See P4269R1. Signed-off-by: romintomasetti <romin.tomasetti@gmail.com>
91e17da to
e43b788
Compare
| template <sender _Sender, __movable_value _Fun> | ||
| constexpr auto operator()(_Sender&& __sndr, _Fun __fun) const -> __well_formed_sender auto | ||
| constexpr auto operator()(_Sender&& __sndr, _Fun __fun) const | ||
| noexcept(__nothrow_decay_copyable<_Fun, _Sender>) |
There was a problem hiding this comment.
We probably need to add std::is_nothrow_move_constructible_v<STDEXEC::__decay_t<_Sender>> here too.
There was a problem hiding this comment.
It seems this would be a departure from the rest of the code base, that usually just checks __nothrow_decay_copyable_t
stdexec/include/stdexec/__detail/__sequence.hpp
Lines 462 to 467 in dd6ca14
This branch has not been deployed
3 participants
exec::fork_join(sndr, clsr)is given a single closure, do no attempt to wrap it in the (thereby useless)fork_joinmachinery. Returnsndr | clsr.exec::fork_join(sndr)is given no closure, it behaves likesndr.As a drive-by, I've added a few
noexcepttothen.Similar to:
See P4269R1.