[boost] [Review.Coroutine] Some comments

Subject: [boost] [Review.Coroutine] Some comments
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-09-04 17:06:09

Hi,

thanks Oliver to move the Coroutine library to a Boost review ready 
proposal. Before doing a formal review, I have some comments, 
suggestions and questions after a quick reading.

Design
====

* I don't know if showing the signature in the documentation will be 
more informative.
This will avoid, in particular, the be forced to explain what T and R 
and in the the current declaration.

Could you replace

template<
     typename  Signature,
     typename  Allocator  =  ctx::stack_allocator
>
class  coroutine
{
public:
     class  self_t
     {
     public:
         T  yield(  R);

         void  yield_break();
     };
by

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

* Could you add the following noexcept specifier?

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.

* As the template class Allocator is not used to allocate internal 
memory of the coroutine class. Could the Allocator parameter be renamed 
StackAllocator?

* Could you add an standard Allocator parameter as e.g. the one of 
packaged_task?

 0.

              template <class F, class Allocator>

               explicit packaged_task(allocator_arg_t, const Allocator& a, F&& f);

C++ International Standard

* I understand that the Boost.Context Allocator parameter is part od the 
class, as context is a low-level class, but coroutine is more high 
level. Is there a possibility to type erase the StackAllocator (and 
preceding Allocator) template parameter, as packaged_task do? This will 
allow to store coroutines (and generators) with the same signature in a 
container even if they don't share the Allocator.

template<typename>class  coroutine;

template<
     typename  R,
     typename  ...ArgTypes
>
class  coroutine<R(ArgTypes...)>
{
public: //...
     template<  typename  Fn,typename  Allocator>
     generator(  Fn  fn,
                std::size_t  size  =  ctx::default_stacksize(),
                flag_unwind_t  do_unwind  =  stack_unwind,
                bool  preserve_fpu  =  true,
                Allocator  alloc  =  Allocator()  );  
//...

};

* Parameters and move semantics. Could the signature parameters be 
rvalue references?

     R  operator()(A0  a0,  ...,  A9  a9);

I'm asking this because I think that Boost.Tuple has not implemented yet move semantics.

* What about moving some of the coroutine/generator optional parameters 
to a coro::attributes class

                std::size_t  size  =  ctx::default_stacksize(),
                flag_unwind_t  do_unwind  =  stack_unwind,
                bool  preserve_fpu  =  true,

* Usually (at least in the C++ standard) a function doesn't throws 
because of its preconditions are not satisfied.

Preconditions:

    |size| > ctx::minimum_stacksize(), |size| < ctx::maximum_stacksize()
    when ! ctx::is_stack_unbound().

Effects:

    Creates a generator which will execute |fn|. If |do_unwind| is
    |stack_unwind| the destructor of |*this| unwinds the stack before
    destructing it. If |preserve_fpu| is |true| the floating-point
    registers are preserved between context switches.

Throws:

    /invalid_argument/ if a precondition is not satisfied.

Should the function throw in this case?

This should be coherent with

          |Result operator()()|
          <http://ok73.ok.funpic.de/boost/libs/coroutine/doc/html/coroutine/generator/generator.html#coroutine.generator.generator._code__phrase_role__identifier__result__phrase___phrase_role__keyword__operator__phrase__phrase_role__special________phrase___code_>

Preconditions:

    |*this| is not a /not-a-generator/, |! is_complete()|.

which doesn't throws any specific exception if the preconditions are not 
satisfied.

.
Documentation
=========

coroutine.html

s/it can migrated between threads/it can migrate between threads/

s/yield_return/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()  );

Best,
Vicente