Re: [boost] [Review.Coroutine] Some comments

Subject: Re: [boost] [Review.Coroutine] Some comments
From: Oliver Kowalke (oliver.kowalke_at_[hidden])
Date: 2012-09-05 02:56:43

> * I don't know if showing the signature in the documentation will be
> more informative.

OK
 
> * Could you add the following noexcept specifier?

OK

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

OK

> * Could you add an standard Allocator parameter as e.g. the one of
> packaged_task?
> 
> 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.

You mean coroutine would have two allocators - one for allocating internal memory and one for allocating the stack?
Removing the StackAllocator from the template arguments of coroutine should not that problem (would require an internal pointer to an internal interface - maybe detail::context_exec is a candidate).
I've some concerns about having two allocator objects in coroutine's ctor
.

> * Parameters and move semantics. Could the signature parameters be
> rvalue references?
> 
>      R  operator()(A0  a0,  ...,  A9  a9);

Hmmm - this would require a second version of opertor() and all arguments are moveable?!

       R operator()(BOOST_RV_REF( A0) a0, ..., BOOST_RV_REF( A)) a9);

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

don't know - would that be an issue

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

instead of passing those threee params as args to coroutine ctor? good point

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

then assert? pre-conditions should be checked but how should I react if I get garbage from the user?

> This should be coherent with
> 
> 
>           |Result operator()()|
>          
> Preconditions:
> 
>     |*this| is not a /not-a-generator/, |! is_complete()|.
> 
> which doesn't throws any specific exception if the preconditions are not
> satisfied.

does an assert - OK the I'll replace throwing excpetions be assertions

> Documentation
> =========
> 
> coroutine.html
> 
> s/it can migrated between threads/it can migrate between threads/

OK

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

OK

I think it will be easy to incorporate your suggestions - but during the boost.context review I was told not to modify the reviewd library (if the review is still on-going).

regards,
Oliver