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

Subject: Re: [boost] [Review.Coroutine] Some comments
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-09-05 07:25:32

Le 05/09/12 08:56, Oliver Kowalke a écrit :
>
>> * 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
> .
I'm suggesting to add a std Allocator as I think the StackAllocator is 
not a model refinement of std Allocator. Am I wrong?

>
>> * 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 not sure. For example, the thread constructor is

     template <class F, class ...Args> explicit thread(F&& f, Args&&... 
args);

and the function F shouldn't define several overloads. I have not 
installed the lib. Maybe you can give a try with a MovableOnly argument 
and see how it behaves.
>
>> I'm asking this because I think that Boost.Tuple has not implemented yet
>> move semantics.
> don't know - would that be an issue.
I think so, because the yield function returns a tuple with the new 
ArgTypes... which should be moved not copied.

         tuple<ArgTypes...>yield(  R);

If ArgTypes is MovableOnly, this will not compile. I don't know if 
fusion::tuple support already Move semantic for their args.
>
>> * 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
Yes.
>
>> * Usually (at least in the C++ standard) a function doesn't throws
>> because of its preconditions are not satisfied.
> then assert?
Yes, I think.
> pre-conditions should be checked but how should I react if I get garbage from the user?
You could use assert.
>
>> 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
Great.
>
> 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).
Well, as you are using Git it should be easy to create a branch, isn't 
it? ;-)

Best,
Vicente