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

Subject: Re: [boost] [Review.Coroutine] More comments, questions and suggestions
From: Oliver Kowalke (oliver.kowalke_at_[hidden])
Date: 2012-09-09 14:21:12

Am 09.09.2012 20:02, schrieb Vicente J. Botet Escriba:
> 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.
OK
> * 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?
It should work even pointers - I've a unit test 
(libs/coroutine/tests/test_coroutine.cpp)
> * 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.
>
yes - works
> * 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).
your mean from self_t::yield()?
>
> 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.
don't know - I could add it but it seams to me syntax sugar
>
> * 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 would not force the user to use self_t::yield() a simple return 
statement does the same.
I don't know if it would be a better interface design - I'm uncertain in 
this point.
in the case of generators - the generators have no signature but 
coroutines do.

>
> * 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.
StackAllocator is not the same as the allocators known from the STL.
StackAllocator return the address from the top or bottom of the 
allocated memory block - depending if the stack grows downwards or upwards.
Anyway - it's  an issue of boost.context.

reagards,
Oliver