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

Subject: Re: [boost] [coroutine][review] Very Late Review
From: Giovanni Piero Deretta (gpderetta_at_[hidden])
Date: 2012-09-18 19:47:11

On Tue, Sep 18, 2012 at 10:45 PM, Eugene Yakubovich
<eyakubovich_at_[hidden]>wrote:

> On Mon, Sep 17, 2012 at 11:24 AM, Giovanni Piero Deretta
> > 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
>

Yes, definitely, get() should return a reference. BTW, I initially thought
that operator* and operator++ would work  well for the coroutine interface,
to mimic the iterator interface, but I think it was a mistake. An explicit
get() is better.

> >
> > 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.
>
>
Help me convince Oliver then ;).
>
> 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.
>

Also technically what it captures is not a full continuation but a one shot
continuation so the name is misleading; the literature uses callcc1 for
them. The name is a bit obscure and it was an half joke anyway.

>
> > 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.
>

It is not unlike an std::function<void(T)> modelling an UnaryFunction and
an std::function<T()> modelling a Generator.

>
> >
> > 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.
>

the continuation in my sketch is movable only:

97: continuation(const continuation&) = delete;

>
> 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.
>

I thought about it, but I couldn't find any way around. As a plus it allows
a few tricks like exiting to a different continuation than the caller.
Anyways, I think it becomes natural after a while. Also note that you do
not really need the explicit move on return in c++11.

-- 
GPD