Some feedback based on integration experience

[glebov-andrey]

Hi,

I've recently been integrating this task implementation into a project which already used a different sender-based coroutine implementation. In doing so, I've come across a number or issues, some of which are related to the specification directly. I've omitted anything already present in P3796R0.

• promise_type::return_value() should probably have a default template parameter to support co_return {...}, i.e.:

template<class V = T>
void return_value(V&& value);

• promise_type::await_transform(change_coroutine_scheduler): the spec uses exchange(SCHED(*this), scheduler_type(sch.scheduler)), but schedulers aren't guaranteed to be move-assignable. Is the intent to disallow such schedulers?
• task::connect() (and task::as_awaitable() in this implementation) should be rvalue-ref-qualified because they move handle
• task_scheduler::ts-sender::connect should probably be rvalue-ref qualified, because there is no guarantee that the wrapped scheduler's sender is const &-connectable. Also, perhaps the spec should say connect(std::move(SENDER(*this)), std::move(r)) (adding std::move() around the wrapped sender)?
• as_awaitable::result (and result_type::result in this implementation): is it a good idea to use std::monostate as an "empty" type. What if the user wants to have their value type be std::monostate? (I know this is a weird edge case, but I actually came across it).

There are also a number of issues with this implementation specifically. I can create a separate issue for them if that would be helpful.

[JeffGarland]

I'm sure @dietmarkuehl will chime in, but a couple thoughts here. If there are specific issues with the impl I'd file those as individual issues -- I'm sure a PR suggesting change is even better :)

is it a good idea to use std::monostate

That change was actually made between version 2 and 3 of the paper which now has:

2 error-variant is a variant<monostate, remove_cvref_t...>, with duplicate types removed, where E... are template arguments of the specialization of execution::completion_signatures denoted by error_types.

I vaguely remember some disucssion of the point, but I don't see anything in the notes. Maybe if there is usage interference it should be looked at as an issue.

[dietmarkuehl]

Hello Andrey,

thank you for the reports. I'll add the specification issues to the next revision of P3796. Most of them seem to be fairly minor:

• Adding a default template argument for return_value() seems like a minor design change and I think it can be applied after C++26 if necessary. I'd hope it can be fixed for C++26, though.
• There is no deliberate intend to prohibit schedulers without move-assignment. I think the requirement can be relax, making the specification more complex, though. It seems it would require destroying the held scheduler and move-constructing it from the parameter.
• Adding r-value qualifications to connect and as_awaitable (either in the implementation or in a modified specification) seems to be reasonable to better indicate what is happening.
• Likewise for ts-sender::connect. With the r-value qualification it should also std::move and objects held during connect.
• The std::monostate is a bit more interesting. My understanding is that std::monostate is intended to indicate the one special state of a std::variant and it isn't intended to be used for anything else. It seems odd if we'd need to create a custom std::monostate every time a special state is needed. I don't think holding a std::variant<...> which is initialised with std::monostate is a problem, just holding the value itself. That said, I don't object to using an exposition-only stopped-state-t and stopped-state to represent the initial state instead.

Fixing these issues in the code should be fairly straight forward. Writing up issues with corresponding proposed fixes should be mostly fairly straight forward, too. Of course, all of that will contribute to the argument that task isn't ready - although after fixing these it is, yet, more ready!

I'd also be interested in an issues with the implementation! They should probably go into one or more separate issues. As Jeff mentioned, PRs would also be welcome but I'm also happy fixing issues! The implementation isn't what I envision by now, yet, either: I expect to do some work on it. In particular, I want to implement some of the proposed solutions from P3796.

[glebov-andrey]

Regarding the std::monostate cases:

• as_awaitable: I've had another look at the latest draft, and now I think everything's fine. Since the specification only uses index-based std::variant functions, it should be safe to store non-unique alternatives.
  The (potential) issue is now only present in the implementation here: https://github.com/bemanproject/execution/blob/fa6d4417c4ba1647b820dba47b03f0d97025cc6c/include/beman/execution/detail/sender_awaitable.hpp#L79-L86
  BTW, the same kind of issue exists if the value type is std::exception_ptr, as weird as that would be.
• promise_type::errors: As far as I can tell, the current wording is still implementable using std::variant<std::monostate, E...>, but it requires reimplementing parts of std::visit (as is done in beman::task::detail::result_type/sub_visit).
  Given that the member is exposition-only and that the way it's accessed is not specified, that should give implementations the freedom to use a custom tag-type (or even another tagged union type entirely) if they prefer. In fact I would hope that implementations would put the value type in the same tagged union to save space.

[dietmarkuehl]

I haven't experimented with it, yet, but since the variant's type has "duplicates removed" I think there may be an issue trying to use monostate as an error type, at least. That is OK but it is also kind of unnecessary: I think I'll add it to the next revision of P3796 with a proposed change in case there is interest.

With respect to the result representation, I think it is probably best to put it into one variant stored in the operation state. To save space, this variant could possibly hold some of the state which becomes unused upon completing the task. I certainly haven't done that, yet. However, as it is all just exposition-only the implementation is free to do whatever it sees fit.