Re: [Boost-users] [boost] [Review.Coroutine] Some comments
* 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
participants (1)
-
Oliver Kowalke