-
Notifications
You must be signed in to change notification settings - Fork 778
[2025-06 LWG Motion 34] P3552R3 Add a Coroutine Task #8032
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[2025-06 LWG Motion 34] P3552R3 Add a Coroutine Task #8032
Conversation
I have applied some [I think editorial] changes compared to P3552r3: - added with_error and change_coroutine_scheduler declarations to the synopsis before task; they are in the detailed description but not in the synopsis (possibly they should only be in the synopsis) as there is no further description) - inconsistent default template arguments for the ctor in the class declaration (allocator<byte> and the later description (allocator<void>); the wording consistently uses allocator<void> - added "of the" to a sentence about the type of a completion datum: "... defines the type *of the* value completion datum" - the use of the template parameter of the nested type state is Rcvr which has two problems: 1. the ctor also uses the name Rcvr for its template parameter 2. the class stores an object of type R which is the receiver named rcvr the most logic fix is to use Rcvr consistently for the state and change the name for the ctor's parameter: 1. change the ctor template parameter type to use R 2. change the member to use Rcvr 3. change the type R used for the own-env-t to be "...get_env(declval<Rcvr>())..."
The CI objected to use of \exposid{sender} => renamed sender to ts-sender. This change should be editorial but is more invovled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for drafting this up.
Lots of work required before this is ready to go.
Also note that fixing up CI builds is not a fixup commit; it should just go into the main commit. The point of fixups is just to distinguish between turning the paper into TeX, and fixing typos and other minor issues afterwards.
Massaging whitespace or paragraph justification is not a separate commit.
whose promise type has an \tcode{unhandled_stopped} member function, or | ||
|
||
\item% | ||
when an exception is thrown from a coroutine \tcode{std::execution::task}\iref{exec.task} which doesn't support a \tcode{std::execution::set_error_t(std::execption_ptr)} completion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when an exception is thrown from a coroutine \tcode{std::execution::task}\iref{exec.task} which doesn't support a \tcode{std::execution::set_error_t(std::execption_ptr)} completion. | |
when an exception is thrown from a coroutine \tcode{std::execution::task}\iref{exec.task} | |
which doesn't support a \tcode{std::execution::set_error_t(std::exception_ptr)} completion. |
This line is overly long, the paper has a typo that needs to be fixed up, and added \iref
s should typically go into a separate fixup commit. I do like the extra \iref
though.
class @\libglobal{task_scheduler}@; | ||
|
||
// \ref{exec.task} | ||
template<class E> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New declarations like these should be in a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What? The incoming paper is missing a declaration? That's at least worth a proper non-fixup "[blah.syn] Add missing declarations" commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As dietmar explained above, those definitions exist, but are missing declarations in the synopsis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. And those declarations seem to have no add-on info, so they probably should exist in the synopsis only.
\tcode{affine_on} adapts a sender into one that completes on | ||
the specified scheduler. If the algorithm determines that the adapted | ||
sender already completes on the correct scheduler it can avoid any | ||
scheduling operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repository uses semantic line breaks (https://sembr.org/). I don't want to bother you too much, but please put new sentences on a separate line at least. That is, newline after .
.
Note that the TeX still renders the same whether you put a space or a newline into the source.
except that \tcode{sndr} is evalutated only once. | ||
|
||
\pnum | ||
The exposition-only class template \exposid{impls-for}\iref{exec.snd.general} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New \iref
s should be in a separate commit.
\begin{codeblock} | ||
namespace std::execution { | ||
template<> | ||
struct @\exposid{impls-for}@<affine_on_t>: @\exposid{default-impls}@ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct @\exposid{impls-for}@<affine_on_t>: @\exposid{default-impls}@ { | |
struct @\exposid{impls-for}@<affine_on_t> : @\exposid{default-impls}@ { |
\tcode{as_awaitable(affine_on(\linebreak std::forward<Sender>(sndr), % avoid Overfull | ||
SCHED(*this)), *this)}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\tcode{as_awaitable(affine_on(\linebreak std::forward<Sender>(sndr), % avoid Overfull | |
SCHED(*this)), *this)}. | |
\tcode{as_awaitable(affine_on(\brk{}std::forward<Sender>(sndr), \exposid{SCHED}(*this)), *this)}. |
Equivalent to: \tcode{returns await_transform(just(exchange(SCHED(*this), | ||
\linebreak scheduler_type(sch.scheduler))), *this);} % avoid Overfull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equivalent to: \tcode{returns await_transform(just(exchange(SCHED(*this), | |
\linebreak scheduler_type(sch.scheduler))), *this);} % avoid Overfull | |
Equivalent to: | |
\begin{codeblock} | |
return await_transform(just(exchange(SCHED(*this), scheduler_type(sch.scheduler))), *this); | |
\end{codeblock} |
Note the typo in returns
(paper has this), but I think this doesn't really need a separate commit.
\pnum | ||
\effects | ||
If the signature \tcode{set_error_t(exception_ptr)} is not an element | ||
of \tcode{error_types}, calls \tcode{terminate()} \iref{except.terminate}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of \tcode{error_types}, calls \tcode{terminate()} \iref{except.terminate}. | |
of \tcode{error_types}, calls \tcode{terminate()}\iref{except.terminate}. |
\tcode{PAlloc} is \tcode{allocator_traits<Allocator>::template | ||
rebind_alloc\linebreak<U>} where \tcode{U} is an unspecified type % avoid Overfull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\tcode{PAlloc} is \tcode{allocator_traits<Allocator>::template | |
rebind_alloc\linebreak<U>} where \tcode{U} is an unspecified type % avoid Overfull | |
\tcode{PAlloc} is \tcode{allocator_traits<Allocator>::template | |
re\-bind_alloc\brk{}<U>} where \tcode{U} is an unspecified type |
coroutine state of size \tcode{size}, and unspecified additional | ||
state necessary to ensure that \tcode{operator delete} can later | ||
deallocate this memory block with an allocator equal to \tcode{palloc}. | ||
\pnum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\pnum | |
\pnum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, pnum needs an empty line in front (or a \begin{itemdescr}, otherwise it's not functioning properly.
@Eisenwave and @dietmarkuehl , let me recap the general rules for paper pull requests, and their rationale.
Adding cross-references to definitions of terms is probably in the "obviously editorial" area, but requires a check whether the xref is pointing to the right place (and thus is putting additional burden on the reviewers). Having a separate, proper editorial commit "[sub.clause.label] Add cross-references to term definitions" would be a bit more transparent, and clearly differentiates paper content from add-on editorial fixes. Occasionally, cross-references are normative (we mean exactly those situations), and changing / amending them would not be editorial. Hope that helps. |
I have applied the changes for P3552r3 (Add a Coroutine Task Type). Compared to the actual text of P3552r3
I made some [I think editorial] changes:
Fixes #7971
Fixes cplusplus/papers#2200