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

Subject: [boost] [Review.Coroutine] Vicente's review
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-09-12 20:26:29

Hi,

I understand that Oliver is busy as all of us and that it is better to provide first the minimal and well designed scope he is confident with. I expect however that Oliver will add soon symmetric coroutines based on the work done by Giovanni in a near future.

- What is your evaluation of the design?

I have some suggestions about changing the interface, but I'm not sure all of them could be implemented in an efficient way.
  
* 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);
     }

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

* 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);
}

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

* 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>());
     }

Or even use some kind of bind parameter

     int f20(coro_t::self_t  &  self)
     {
               int i;
               int j;
               self::bind(i, j);

         self.yield( i + j );
     }

     int f20()
     {
               int i;
               int j;
               typedef this_coroutine<int(int)> this_coro;
               this_coro::bind(i, j);

               this_coro::yield( i + j );

     }

If bind is adopted, the implementation could use it to store directly the actual parameters, instead of needing to store them in the self_t instance and
them returning them when yield is called.

*/As//  Boost.Coroutine is able to transfer user exceptions, and there is no difference between calling to coroutine<>::self_t::yield_break()/  
and throwing/coroutine_terminated,/both (/coroutine<>::self_t::yield_break()/  and throwing/coroutine_terminated)/be removed from the library.
     
The same applies for generators.

* noexcept specifier should be added to

class  coroutine
{ // ...
     coroutine() BOOST_NOEXCEPT;     coroutine(  coroutine  &&  other)  BOOST_NOEXCEPT;
     coroutine  &  operator=(  coroutine  &&  other)  BOOST_NOEXCEPT;
     explicitoperator  bool()  const  BOOST_NOEXCEPT;
     bool  is_complete()  const  BOOST_NOEXCEPT;
//...
};

void  swap(  coroutine  &  l,  coroutine  &  r) BOOST_NOEXCEPT;

The same applies to generator.

* Type-erase on the Allocator parameter as the standard std::packaged_task does.

* Make coroutine/generator classes to follow the allocator aware standard protocol adding the constructor with the standard allocator. A specializations of the use_allocator template should be added for this purpose.

* Moving some of the coroutine/generator optional parameters to a coro::attributes class could simplify the interface
     
                std::size_t  size  =  ctx::default_stacksize(),
                flag_unwind_t  do_unwind  =  stack_unwind,
                bool  preserve_fpu  =  true,

* The lib shouldn't throw when preconditions are not satisfied, assert should instead.
  
* It is a shame that the library can not support completely move semantics for the coroutine parameters, as the used boost::tuple don't support it yet. I would like this point be addressed before the first release so that there is no break in the interface when this will be solved either using boost::tuple, boost::fusion::tuple or std::tuple. At a last resort the library could either define a macro that allows to parameterize the tuple implementation, or alternatively it could implement a wrapper that forwards to the best implementation.

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

- 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 and the use of the pre-processor to manage with variadic templates hides a possible good design (as it is usual in those cases). Maybe now that most of the c++ compilers support them, using the preprocessor is less imperative.

- What is your evaluation of the documentation?

I find the documentation a little bit sparse and contain a lot or typos and minor errors. I would appreciate if the original Boost.Coroutine documentation from Giovannii were not lost, at least for Motivation and Tutorial parts.

I would appreciate also if the documentation include the rationale to don't include symmetric coroutines and show how the current library can be used to solve the problems the missing features provide, why not as examples.

* showing the signature in the documentation will be more informative.

template<     
     typename  Signature,
     typename  Allocator  =  ctx::stack_allocator
>class  coroutine;
template<
     typename  R,
     typename  ...ArgTypes,
     typename  Allocator
  >
class  coroutine<R(ArgTypes...)>
{
public:
     class  self_t
     {
     public:
         tuple<ArgTypes...>yield(  R);
         void  yield_break();
     };

* documentation for the following constructor is missing in the reference
     
         template<  typename  Fn  >
     coroutine(  Fn  &&  fn,
                std::size_t  size  =  ctx::default_stacksize(),
                flag_unwind_t  do_unwind  =  stack_unwind,
                bool  preserve_fpu  =  true,
                Allocator  alloc  =  Allocator()  );

*The example

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

void  f(  coro_t::self_t  &  self,  int  i)
{
     ...
     self.yield();
     ...
}

coro_t  c(  boost::bind(  f,  _1)  );
c(  7);

doesn't compiles. It should be something like

     coro_t c( boost::bind( f, _1, _2) );

BTW, I don't see the need to use boost::bind in this case, as

     coro_t c( f );

works as well.

The superfluous uses of bind in the documentation should be removed:

- What is your evaluation of the potential usefulness of the library?

Very useful. coroutines and generators are some useful constructions that are missing in the language.

- Did you try to use the library?  With what compiler?  Did you have any
problems?

Yes. I used gcc-4.2.1,4.6.0, 4.7.0,clang2.9 andclang-3.0 on darwin. I had no problem running the provided examples and tests.
I have written some examples.

   - How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I have read the documentations of Oliver and Giovanii libraries, followed the review and used the library.

  - Are you knowledgeable about the problem domain?

 From the theoretical point of view, yes. Never used in practice.

Finally, my vote is YES. Even if the current interface has some drawbacks that should be mentioned in the documentation, I think I Boost.Coroutine is already useful and should be accepted into Boost.

Best,
Vicente

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