Re: [boost] [Review.Coroutine] Vicente's review

Subject: Re: [boost] [Review.Coroutine] Vicente's review
From: Oliver Kowalke (oliver.kowalke_at_[hidden])
Date: 2012-09-13 02:47:21

> * The use of thecoro_t::self_t  &  self  parameter makes too much noise.
> This parameter must be forwarded to any function that could yield the
> coroutine.
> 
> An alternative design using a thread specific pointer could allow to
> access the current coroutine
> 
>      int f20()
>      {
> 	typedef this_coroutine<int(void)> this_coro;
>          this_coro::yield(5);
>      }

I was also thinking about this issue (as I already implemented it in boost.fiber) but how do I know in f20() which coroutine is active (f20() is bound to)? In the case of boost.fiber I get this info from the internal scheduler but boost.coroutine hasn't one (and shouldn't).

> Is the rationale for using self_t to get better performances?
> Maybe both interfaces could be provided.

At least it would introduce additional indirections.

> * There is an asymmetry between the value returned the last time and the
> others.
> 
>      self.yield(  i); // OTHERS
>      return  i; // LAST
> 
> This asymmetry forces the user to manage the last return in a specific
> case, changing the readability of the code. I would prefer coroutine function
> returns void.
> voidfn(  coro_t::self_t  &  self)
> {
>      self.yield(1);
>      self.yield(2);
> }

generator<> now requires a return type of 'void' of the generator function.
This makes sense because generator< R >::operator() returns a optional< R > (Eugene's suggestion).

generator< int > gen( f);
if ( optional< int > val = gen() ) {...}

if the generator function simple returns (no yield() called) then gen() returns a none (== invalid/unset optional).

In the case of coroutines coroutine<R>::operator() should still return R (not optional<R>).

void f( self_t &) {}
coroutine< string( int, int) > coro( f);
string s = coro( 1, 2);

Which value should coroutine< string >::operator() return if the coroutine-function is required to return void and the user simply returns without calling yield()?

My impression is that this asymmetry between coroutines and generators should be kept. 

* If this is not adopted, the implementation should check at compile time
> that the return type of the coroutine function is the one of the signature.

done for coroutine and generators (prevent also void as return type for generators).

> * The access to the actual coroutine parameters is asymmetric, letting
> access to old actual parameters that can point to objects that have already
> been destroyed.
> 
> Maybe the self_t object could take care of this (or this_coroutine).  We
> can use a get<> function to give access to the actual parameters.
> 
>      int f20(coro_t::self_t  &  self)
>      {
>          self.yield(2*self.get<0>());
>      }
> 
> 
>      int f20()
>      {
> 	typedef this_coroutine<int(int)> this_coro;
>          this_coro::yield(2*this_coro::get<0>());
>      }

interesting idea, I find it better than the version using bind because you don't can't forget to 'bind'. you are always required to use self_t::get<>().

> * As others have noted, providing a view of a generator as an output
> iterator will be welcome. Viewing it as a range will be also appreciated. BTW, I
> don't like the alternative using optional as the result of a generator.
> Following the output iterator concept the access is defined only if the
> generator has not finished.

hmm - I've already adopted returning optional. couldn't the iterator build on top of the current interface?

generator< X > gen( f);
generator_iterator< X > i( move( gen) );
generator_iterator< X > e();

for (; i!= e; ++i) {
   * i;
}

> - What is your evaluation of the implementation?
> 
> I have not a deep opinion as I have not found the time to inspect it. At
> first glance, it seems that the inheritance hierarchy is deeper than needed

the code uses static polymorphism (curiously recurring template pattern), that means that you don't pay overhead.
I was forced to split up the code into separate templates because of too much template specialization == specialize return type (separate void), specialize amount of function parameters -> defines coroutine<>::operator()( Arg0 ,...).

> P.S. Maybe I have forgotten to include some of the remarks I did on
> preceding posts.

I've most of your suggestions already implemented.

regards,
Oliver