[Review.Coroutine] More comments, questions and suggestions
Hi, I understand Oliver that you are busy as all of us and that it is better to provide first the minimal scope you are confident with. I expect however that you will add symmetric coroutines based on the work done by Giovanni in a near future. BTW, adding a link to the Giovanni documentation will be welcome. I have more questions concerning the interface before finishing my review: * How the library behaves when the coroutine parameters are const& typedef boost::coro::coroutine< int( AType const& ) > coro_t; int fn( coro_t::self_t & self, AType const& v) { ???j = self.yield( v.get_int() ); /// ... } What should be the type (???) of j? Does AType const& works? * Have you tested with other kind of parameters? arrays, pointers, ... It would be great if the documentation includes some examples, with the different types that can be used. * The way the parameters of the coroutine call are retrieved the second time is a little bit disturbing at a first glance, but once you understand how this works, it is not so complex (This applies to the library under review and the original one, as both share the same design). I don't know if the the self object couldn't store the parameter binding, so that the user will not be concerned by the yield result type. typedef boost::coro::coroutine< int( int) > coro_t; int fn( coro_t::self_t & self, int i) { // binds current parameters to the self object. Done only once. self.bind(i); // *** std::cout << "fn(): local variable i ==" << i << std::endl; // save current coroutine context // transfer execution control back to caller // no need to assign the result. It is up to yield to fill the binded parameters. self.yield(i); // OTHERS *** // i == 10 because c( 10) in main() and i has been bound in self. std::cout << "fn(): local variable i ==" << i<< std::endl; return i; // LAST } I don't know this bind design introduce more problems that it could solve. I don't know neither if this could degrade the performances. * In the preceding example there is an asymmetry between the value returned the last time and the others. self.yield( i); // OTHERS return i; // LAST What about forcing the coroutine function return void, as it is the case now for the generator? voidfn( coro_t::self_t & self, int i) { self.bind(i); std::cout << "fn(): local variable i ==" << i << std::endl; self.yield(i); std::cout << "fn(): local variable i ==" << i<< std::endl; self.yield(i); } This should help to simplify user code, isn't it? * I don't know if you have reached to make the StackAllocator standard compliant, but if this is the case you could add also a specializations of the use_allocator template so that Boost.Container can take it in account. Best, Vicente
Am 09.09.2012 20:02, schrieb Vicente J. Botet Escriba:
Hi,
I understand Oliver that you are busy as all of us and that it is better to provide first the minimal scope you are confident with. I expect however that you will add symmetric coroutines based on the work done by Giovanni in a near future. BTW, adding a link to the Giovanni documentation will be welcome. OK * How the library behaves when the coroutine parameters are const&
typedef boost::coro::coroutine< int( AType const& ) > coro_t;
int fn( coro_t::self_t & self, AType const& v) { ???j = self.yield( v.get_int() ); /// ... } What should be the type (???) of j? Does AType const& works? It should work even pointers - I've a unit test (libs/coroutine/tests/test_coroutine.cpp) * Have you tested with other kind of parameters? arrays, pointers, ... It would be great if the documentation includes some examples, with the different types that can be used.
* The way the parameters of the coroutine call are retrieved the second time is a little bit disturbing at a first glance, but once you understand how this works, it is not so complex (This applies to the library under review and the original one, as both share the same design). your mean from self_t::yield()?
I don't know if the the self object couldn't store the parameter binding, so that the user will not be concerned by the yield result type.
typedef boost::coro::coroutine< int( int) > coro_t;
int fn( coro_t::self_t & self, int i) { // binds current parameters to the self object. Done only once. self.bind(i); // ***
std::cout << "fn(): local variable i ==" << i << std::endl;
// save current coroutine context // transfer execution control back to caller // no need to assign the result. It is up to yield to fill the binded parameters. self.yield(i); // OTHERS *** // i == 10 because c( 10) in main() and i has been bound in self. std::cout << "fn(): local variable i ==" << i<< std::endl;
return i; // LAST }
I don't know this bind design introduce more problems that it could solve. I don't know neither if this could degrade the performances. don't know - I could add it but it seams to me syntax sugar
* In the preceding example there is an asymmetry between the value returned the last time and the others.
self.yield( i); // OTHERS return i; // LAST
What about forcing the coroutine function return void, as it is the case now for the generator?
voidfn( coro_t::self_t & self, int i) { self.bind(i); std::cout << "fn(): local variable i ==" << i << std::endl;
self.yield(i); std::cout << "fn(): local variable i ==" << i<< std::endl; self.yield(i);
}
This should help to simplify user code, isn't it? I would not force the user to use self_t::yield() a simple return statement does the same. I don't know if it would be a better interface design - I'm uncertain in
yes - works this point. in the case of generators - the generators have no signature but coroutines do.
* I don't know if you have reached to make the StackAllocator standard compliant, but if this is the case you could add also a specializations of the use_allocator template so that Boost.Container can take it in account.
StackAllocator is not the same as the allocators known from the STL. StackAllocator return the address from the top or bottom of the allocated memory block - depending if the stack grows downwards or upwards. Anyway - it's an issue of boost.context. reagards, Oliver
Le 09/09/12 20:21, Oliver Kowalke a écrit :
Am 09.09.2012 20:02, schrieb Vicente J. Botet Escriba:
* How the library behaves when the coroutine parameters are const&
typedef boost::coro::coroutine< int( AType const& ) > coro_t;
int fn( coro_t::self_t & self, AType const& v) { ???j = self.yield( v.get_int() ); /// ... } What should be the type (???) of j? Does AType const& works? It should work even pointers - I've a unit test (libs/coroutine/tests/test_coroutine.cpp)
I have installed Boost.Coroutine on the release branch and everything compiles and run :). I have tried it with std::string const&. int f77( coro_void_string::self_t & self, std::string const& str) { std::string const& str2 = self.yield( str.length()); return str2.length(); } void test_arg_string2() { value2 = ""; coro_int_string coro( boost::bind( f77, _1, _2) ); BOOST_CHECK( ! coro.is_complete() ); int r = coro( std::string("abc") ); BOOST_CHECK( ! coro.is_complete() ); BOOST_CHECK_EQUAL( r, 3); r = coro( std::string("") ); BOOST_CHECK( coro.is_complete() ); BOOST_CHECK_EQUAL( r, 0); } Of course I can not define it as int f77( coro_int_string::self_t & self, std::string const& str) { str = self.yield( str.length()); // COMPILE ERROR return str.length(); } as the reference can not be reassigned. I would really like to be able to use the same name to access the coroutine parameter during the whole scope of the coroutine. The single way I see of been able to reuse the same name is declaring a new variable that can be reassigned each time the yield function returns. Of course this variable can not be a reference, but a pointer. int f78( coro_int_string::self_t & self, std::string const& str_dont_use_it) { std::string const* str = &str_dont_use_it; // and reassign it each time yield is called str = &self.yield( str->length()); str = &self.yield( str->length()); return str->length(); } While this is not satisfactory, maybe the self_t could take care of it. int f78( coro_int_string::self_t & self, std::string const& str_dont_use_it) { std::string const* str; self.bind(str, str_dont_use_it); // now self_t will reassign to str each time yield is called self.yield( str->length()); self.yield( str->length()); return str->length(); } This is close to the goal I'm looking for. The user should not take care of reassigning the actual parameters. Oliver, I don't know if you , that know well your library, see if this could be implemented or even improved. Best, Vicente P.S. Note that all this issues appear only with references, which can not be reassigned. For non-reference parameters I expect the user to reuse the parameter itself.
While this is not satisfactory, maybe the self_t could take care of it.
int f78( coro_int_string::self_t & self, std::string const& str_dont_use_it) { std::string const* str; self.bind(str, str_dont_use_it);
// now self_t will reassign to str each time yield is called self.yield( str->length()); self.yield( str->length()); return str->length(); }
This is close to the goal I'm looking for. The user should not take care of reassigning the actual parameters.
Oliver, I don't know if you , that know well your library, see if this could be implemented or even improved.
I've to think about how this issue could be solved - might take a while. regards, Oliver
Le 09/09/12 20:21, Oliver Kowalke a écrit :
Am 09.09.2012 20:02, schrieb Vicente J. Botet Escriba:
* The way the parameters of the coroutine call are retrieved the second time is a little bit disturbing at a first glance, but once you understand how this works, it is not so complex (This applies to the library under review and the original one, as both share the same design). your mean from self_t::yield()? Yes.
I don't know if the the self object couldn't store the parameter binding, so that the user will not be concerned by the yield result type.
typedef boost::coro::coroutine< int( int) > coro_t;
int fn( coro_t::self_t & self, int i) { // binds current parameters to the self object. Done only once. self.bind(i); // ***
std::cout << "fn(): local variable i ==" << i << std::endl;
// save current coroutine context // transfer execution control back to caller // no need to assign the result. It is up to yield to fill the binded parameters. self.yield(i); // OTHERS *** // i == 10 because c( 10) in main() and i has been bound in self. std::cout << "fn(): local variable i ==" << i<< std::endl;
return i; // LAST }
I don't know this bind design introduce more problems that it could solve. I don't know neither if this could degrade the performances.
don't know - I could add it but it seams to me syntax sugar Maybe, but not having to store explicitly the new parameter values seems a good improvement, if doable. In addition 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.
Thoughts?
* In the preceding example there is an asymmetry between the value returned the last time and the others.
self.yield( i); // OTHERS return i; // LAST
What about forcing the coroutine function return void, as it is the case now for the generator?
voidfn( coro_t::self_t & self, int i) { self.bind(i); std::cout << "fn(): local variable i ==" << i << std::endl;
self.yield(i); std::cout << "fn(): local variable i ==" << i<< std::endl; self.yield(i);
}
This should help to simplify user code, isn't it?
I would not force the user to use self_t::yield() a simple return statement does the same. The current interface forces the user to return the last value using 'return', which seems no better to me. Or, would this perform better?
BTW, I understand that the following coroutine returns twice, but it is a little bit obscure, as there are two ways to return a value from a coroutine. int f( coro_ref::self_t & self, int a) { return self.yield( a); } LUA follows the same schema, but I don't share it. I you adopt the same design as generators there will be no possible confusion. Only one way to yield a value. What do others think?
I don't know if it would be a better interface design - I'm uncertain in this point. in the case of generators - the generators have no signature but coroutines do.
* I don't know if you have reached to make the StackAllocator standard compliant, but if this is the case you could add also a specializations of the use_allocator template so that Boost.Container can take it in account.
StackAllocator is not the same as the allocators known from the STL. StackAllocator return the address from the top or bottom of the allocated memory block - depending if the stack grows downwards or upwards. Anyway - it's an issue of boost.context.
Sorry. I believed you had reached to mix them. Maybe you could reach to make a StackAllocator model a standard allocator just by renaming the stack allocator functions. This will break existing code defining its own stack allocator but the change seems minor to me. Best, Vicente
What about forcing the coroutine function return void, as it is the case now for the generator?
voidfn( coro_t::self_t & self, int i) { self.bind(i); std::cout << "fn(): local variable i ==" << i << std::endl;
self.yield(i); std::cout << "fn(): local variable i ==" << i<< std::endl; self.yield(i);
}
This should help to simplify user code, isn't it?
I would not force the user to use self_t::yield() a simple return statement does the same.
The current interface forces the user to return the last value using 'return', which seems no better to me. Or, would this perform better?
BTW, I understand that the following coroutine returns twice, but it is a little bit obscure, as there are two ways to return a value from a coroutine.
int f( coro_ref::self_t & self, int a) { return self.yield( a); }
LUA follows the same schema, but I don't share it. I you adopt the same design as generators there will be no possible confusion. Only one way to yield a value.
What do others think?
That's an interesting idea. If you couple it with the argument I made few mins ago regarding not invoking generator-function in generator's constructor, the coroutines and generators start to look very similar. The only difference would basically be that generators don't have arguments (yield returns void). But then maybe we can learn something from Python here. They also started with pure generators (sending values one way) but eventually extended them to be bidirectional. Maybe we can unite coroutine and generator into just one class? Regards, Eugene
BTW, I understand that the following coroutine returns twice, but it is a little bit obscure, as there are two ways to return a value from a coroutine.
int f( coro_ref::self_t & self, int a) { return self.yield( a); }
LUA follows the same schema, but I don't share it. I you adopt the same design as generators there will be no possible confusion. Only one way to yield a value.
What do others think?
It is possible to implement it but I've concerns because the coroutine-fn has another signature/return type as the signature given to coroutine as template argument. It might confuse users. typedef coroutine< int( int) > coro_t; void fn( coro_t::self_t&, int) {...}
Sorry. I believed you had reached to mix them. Maybe you could reach to make a StackAllocator model a standard allocator just by renaming the stack allocator functions.
standard-allocator requires functions like deallocate/destory allocate/create. stack-allocators do not construct objects the allocate/deallocate only memory chunks. I believe those are two different concepts (beside the special requirements of stack-allocators on the return address of the allocated chunk). Oliver
On Tuesday 11 September 2012 09:08:03 Oliver Kowalke wrote:
It is possible to implement it but I've concerns because the coroutine-fn has another signature/return type as the signature given to coroutine as template argument. It might confuse users.
typedef coroutine< int( int) > coro_t;
void fn( coro_t::self_t&, int) {...}
I don't follow the thread closely, but you could provide a placeholder type to indicate the self_t argument in the signature. Then typedef coroutine< int(self, int) > coro_t; or typedef coroutine< int(int, self) > coro_t; or typedef coroutine< int(int) > coro_t; are all possible and self-explaining.
On Tuesday 11 September 2012 09:08:03 Oliver Kowalke wrote:
It is possible to implement it but I've concerns because the
coroutine-fn
has another signature/return type as the signature given to coroutine as template argument. It might confuse users.
typedef coroutine< int( int) > coro_t;
void fn( coro_t::self_t&, int) {...}
I don't follow the thread closely, but you could provide a placeholder type to indicate the self_t argument in the signature.
I mean the return type of coroutine-fn and the signature int htecoroutine template arg differ (beside the self_t argument).
Oliver Kowalke wrote
BTW, I understand that the following coroutine returns twice, but it is a little bit obscure, as there are two ways to return a value from a coroutine.
int f( coro_ref::self_t & self, int a) { return self.yield( a); }
LUA follows the same schema, but I don't share it. I you adopt the same design as generators there will be no possible confusion. Only one way to yield a value.
What do others think?
It is possible to implement it but I've concerns because the coroutine-fn has another signature/return type as the signature given to coroutine as template argument. It might confuse users.
typedef coroutine< int( int) > coro_t;
void fn( coro_t::self_t&, int) {...}
Does the current implementation check at compile time that the return type of the coroutine function is the one of the signature?
Sorry. I believed you had reached to mix them. Maybe you could reach to make a StackAllocator model a standard allocator just by renaming the stack allocator functions.
standard-allocator requires functions like deallocate/destory allocate/create.
stack-allocators do not construct objects the allocate/deallocate only memory chunks. I believe those are two different concepts (beside the special requirements of stack-allocators on the return address of the allocated chunk).
Yes. It is a bad idea to mix them. I don't remember if you plan to add standard allocator or not. Could you confirm? Best, Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Review-Coroutine-More-comments-questions-... Sent from the Boost - Dev mailing list archive at Nabble.com.
It is possible to implement it but I've concerns because the coroutine-fn has another signature/return type as the signature given to coroutine as template argument. It might confuse users.
typedef coroutine< int( int) > coro_t;
void fn( coro_t::self_t&, int) {...}
no sure - probably not, but it's not that hard to add an BOOST_STATIC_ASSERT :)
Does the current implementation check at compile time that the return type of the coroutine function is the one of the signature?
Sorry. I believed you had reached to mix them. Maybe you could reach to make a StackAllocator model a standard allocator just by renaming the stack allocator functions.
standard-allocator requires functions like deallocate/destory allocate/create.
stack-allocators do not construct objects the allocate/deallocate only memory chunks. I believe those are two different concepts (beside the special requirements of stack-allocators on the return address of the allocated chunk).
Yes. It is a bad idea to mix them. I don't remember if you plan to add standard allocator or not. Could you confirm?
yes - the version in my git-repo uses allocators (std::allocator as default) in coroutines ctor (beside of the stack-allocator). regards, Oliver
Hi, just two minor additional comments: 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 bind in this case coro_t c( f ); works as well. Could you remove the superfluous uses of bind in the documentation? Best, Vicente
Hi, One more question /I don't know if this has already be commented. Now that Boost.Coroutine is able to transfer user exceptions, what is the difference between calling to coroutine<>::self_t::yield_break()/ and throwing /coroutine_terminated?/ If there is no one, shouldn't both (/coroutine<>::self_t::yield_break()/ and throwing /coroutine_terminated) /be removed from the library? The same applies for generators. Best, Vicente
Now that Boost.Coroutine is able to transfer user exceptions, what is the difference between calling to coroutine<>::self_t::yield_break()/ and throwing /coroutine_terminated?/
equivalent - in previous implementations yield_break() adjusted some internal flags (now done in another way)
If there is no one, shouldn't both (/coroutine<>::self_t::yield_break()/ and throwing /coroutine_terminated) /be removed from the library?
The same applies for generators.
generator<>::self_t::yield_break() was removed Oliver
participants (5)
-
Andrey Semashev -
Eugene Yakubovich -
Oliver Kowalke -
Vicente Botet -
Vicente J. Botet Escriba