Re: [boost] [coroutine][review] Very Late Review

Subject: Re: [boost] [coroutine][review] Very Late Review
From: Eugene Yakubovich (eyakubovich_at_[hidden])
Date: 2012-09-18 17:45:40

On Mon, Sep 17, 2012 at 11:24 AM, Giovanni Piero Deretta
<gpderetta_at_[hidden]> wrote:
>
> About the design:

I want to comment on this proposed design but not argue if it should
replace what is currently under review.

> I like the idea that has been proposed during the review that the result of
> a coroutine should be retrivable at any time via a get method. I dislike
> the proposed optional<T> result for operator(). If the previous delayed
> result functionality is implemented, operator() should just return *this,
> so that the subsequent get() call can be chained or the result tested for
> non empty. This change has the potential of deeply affecting the interface,
> so I would really like to see it subject to a mini review before acceptance.

In this case, get() or operator* (like in your sketch impl) should
return T&, not T. Otherwise you get a surprise move:
T x = *c; // OK, value has been moved into x.
T y = *c; // y contains an empty value

>
> It has been suggested that coroutine::self be removed an have a thread
> specific object. I strongly disagree with this option. First of all it will
> make it too easy to break type safety:
>
> typedef this_coroutine<int(int)> this_coro;
>
> typedef this_coroutine<void()> that_coro;
>
> coroutine<int(int)>([]{
>        that_coro::yield(); // oops compiles fine, UB at runtime.
> });
>
This is a good point. I now think I'd prefer type safety here so I
agree that using a thread specific object is bad.

> Instead I suggest doing away with a separate self type and instead making
> the first parameter to the coroutine functor a full fledged coroutine, with
> simmetric signature:
>
> coroutine<int(void)> my_coroutine([](coroutine<void(int)> caller) {
>               caller(10);
> });
>
> int i = my_coroutine();

I like this.

>
> This means that the parameter no longer represent the coroutine itself,
> but, more logically, the caller coroutine. This makes very obvious the
> analogy between coroutines and channels. It also blurs the division between
> symmetric and asymmetric coroutines. Bonus points if you had a function
> that infers the coroutine singnature from the coroutine functor operator()
> (I like to call this function callcc for obvious reasons [
> http://en.wikipedia.org/wiki/Call-with-current-continuation]):
>
> auto my_coroutine = callcc([](coroutine<void(int)> caller) { ... } );
>

I agree that there're similarities with call/cc but I'm not sure it's
exactly the same. call/cc returns the "yielded" value, not a
coroutine. It only returns a coroutine if the "yield" was bundled with
another call to call/cc. So to avoid the confusion, a different name
might be best.

> I do not like the separation between coroutines and generators. Instead the
> generator class should be dropped. The reason that I had generator as a
> separate class in the original coroutine SoC is because I wanted to give it
> iterator semantics (this also required internally using a reference counted
> pointer to make the iterator copiable, which I disliked). Instead I propose
> to make every coroutine<T()> fulfill the the input range concept and every
> coroutine<void(T)> fulfill the output range concept (this is a
> functionality not covered by generators).

While I like this suggestion, I worry that having one template
specialization model one concept and another specialization model
another concept (Input and Output iterators) might be too confusing.

>
> The last suggestion works very well if coupled with the suggestion of
> making 'self' a full coroutine:
>
> std::vector<int> a = { ... };
> std::vector<int> b = { ... };
>
> std::cout << "The result of 'merge(a,b) is:\n";
> for(auto x : callcc([&](coroutine<void(int)> c) {  std::merge(a.begin(),
> a.end(), b.begin(), b.end(), c.begin()); })
> {
>         std::cout << x << "\n";
> }
>
This does look compelling.

Also, the sketch implementation's continuation class is copyable.
Conceptually, continuation/coroutine encompasses a resource (context)
and should be movable only (like other resources: e.g. file handle
wrappers).

It would also be great if the coroutine function could return void
instead of making the user return (moved) coroutine. I understand why
it's needed from the implementation point of view but I wonder if
there's a way around it.

Regards,
Eugene