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

Subject: Re: [boost] [Review.Coroutine] Vicente's review
From: Vicente Botet (vicente.botet_at_[hidden])
Date: 2012-09-13 10:42:50

Oliver Kowalke wrote
> 
>> * 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).
> 

A coroutine runs on the same thread as its caller. The call can reset the
self_t instance on a thread specific pointer so that it can be retrieve
later on.
 

>> Is the rationale for using self_t to get better performances?
>> Maybe both interfaces could be provided.
> 
> At least it would introduce additional indirections.
> 

I agree that a thread specific pointer is not gratuitous, but adding the
self_t parameter over the whole stack has its cost also.

Providing both approaches would let the user choose which one is best
adapted to her context.

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

I don't think optional should be used. The user must know if it can call the
generator to get the next value or not. 

My point was independent of the generator interface. I don't think the
asymmetry is beneficial, but maybe I'm missing something important.

> 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()?
> 

A coroutine should yield a value once it is called/resumed. The library can
check that a value has been yield when the function returns, isn't it?

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

I would like to be convinced. For the time been I'm not.

>> * 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&lt;int(int)&gt; 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;
> }
> 

The iterator interface don't need the use of optional. It need just to know
if the generator has completed or not and only when not completed been able
to retrieve the value.

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

I can understand your argument.

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

Great.

Best.
Vicente

--
View this message in context: http://boost.2283326.n4.nabble.com/Review-Coroutine-Vicente-s-review-tp4635672p4635692.html
Sent from the Boost - Dev mailing list archive at Nabble.com.