[boost] [Review.Coroutine] More comments, questions and suggestions

Subject: [boost] [Review.Coroutine] More comments, questions and suggestions
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-09-09 14:02:35

Hi,

I understand Oliver that you are busy as all of us and that it is better 
to provide first the minimal scope you are confident with. I expect 
however that you will add symmetric coroutines based on the work done by 
Giovanni in a near future.  BTW, adding a link to the Giovanni 
documentation will be welcome.

I have more questions concerning the interface before finishing my review:

* How the library behaves when the coroutine parameters are const&

typedef  boost::coro::coroutine<  int(  AType  const& ) >     coro_t;

int  fn(  coro_t::self_t  &  self,  AType  const&  v)
{
     ???j  =  self.yield(  v.get_int()  );
     /// ...
}
What should be the type (???) of j? Does AType  const&  works?

* Have you tested with other kind of parameters? arrays, pointers, ... 
It would be great if the documentation includes some examples, with the 
different types that can be used.

* The way the parameters of the coroutine call are retrieved the second 
time is a little bit disturbing at a first glance, but once you 
understand how this works, it is not so complex (This applies to the 
library under review and the original one, as both share the same design).

I don't know if the the self object couldn't store the parameter 
binding, so that the user will not be concerned by the yield result type.

typedef  boost::coro::coroutine<  int(  int)  >     coro_t;

int  fn(  coro_t::self_t  &  self,  int  i)
{
     // binds current parameters to the self object. Done only once.
     self.bind(i); // ***

     std::cout  <<  "fn(): local variable i =="  <<  i <<  std::endl;

     // save current coroutine context
     // transfer execution control back to caller
     // no need to assign the result. It is up to yield to fill the 
binded parameters.
     self.yield(i); // OTHERS  ***
     // i == 10 because c( 10) in main() and i has been bound in self.
     std::cout  <<  "fn(): local variable i =="  << i<<  std::endl;

     return  i; // LAST
}

I don't know this bind design introduce more problems that it could 
solve. I don't know neither if this could degrade the performances.

* In the preceding example there is an asymmetry between the value 
returned the last time and the others.

     self.yield(  i); // OTHERS
     return  i; // LAST

What about forcing the coroutine function return void, as it is the case 
now for the generator?

voidfn(  coro_t::self_t  &  self,  int  i)
{
     self.bind(i);
     std::cout  <<  "fn(): local variable i =="  <<  i <<  std::endl;

     self.yield(i);
     std::cout  <<  "fn(): local variable i =="  << i<<  std::endl;
     self.yield(i);

}

This should help to simplify user code, isn't it?

* I don't know if you have reached to make the StackAllocator standard 
compliant, but if this is the case you could add also a specializations 
of the use_allocator template so that Boost.Container can take it in 
account.

Best,
Vicente