{Review] Coroutine reviewstarts today, September 3rd
Hi all, The review of Oliver Kowalke's Boost.Coroutine library starts today, September 3rd 2012, and will end on September 12th. I really hope to see your vote and your participation in the discussions on the Boost mailing lists! ----------------------------------------------------- About the library: Boost.Coroutine provides templates for generalized subroutines (coroutines) which allow multiple entry points for suspending and resuming execution at certain locations. It preserves local state/data and allows re-entering subroutines more than once (useful if state must be kept across function calls). Coroutines can be viewed as a language-level construct providing a special kind of control flow. You can download the library and the view documentation here: docs: http://ok73.ok.funpic.de/boost/libs/coroutine/doc/html/ src: http://ok73.ok.funpic.de/boost.coroutine.zip --------------------------------------------------- Please always state in your review, whether you think the library should be accepted as a Boost library! Additionally please consider giving feedback on the following general topics: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? Regards Hartmut Review Manager
On Mon, Sep 3, 2012 at 6:46 AM, Hartmut Kaiser <hartmut.kaiser@gmail.com> wrote:
Hi all,
The review of Oliver Kowalke's Boost.Coroutine library starts today, September 3rd 2012, and will end on September 12th. I really hope to see your vote and your participation in the discussions on the Boost mailing lists!
Hi Oliver, I'm new to this list but because I wanted a coroutine library for a long time, I want to participate in this review. I'm also very impressed by Boost.Context and glad that it stands as a standalone and not in the detail namespace of this coroutine library. I have a few questions, though. Starting with generator class (easiest): 1. What is the rationale for having the generator-function's return statement be used to return the last generated value or forcing the use of yield_break (which uses exceptions that can be slow). My only experience with generators/coroutines has been in Python which might have biased my view but I think having the generator-function return void would be more natural (and would eliminate the need for yield_break). For example, if I wanted to generate a sequence of evens, I could write: typedef boost::coro::generator< int > gen_t; gen_t evens(int cnt) { return [=](gen_t::self_t& self) { for( int i = 0; i < cnt; i++ ) self.yield(i*2); }; } 2. Considering that generators are used to produce a sequence of values, would it make sense to support range based for loop? While playing with the library, I defined a (hacky) iterator together with begin/end functions that made it possible to: for( int x: evens(10) ) std::cout << x << ' '; I found such usage very natural and wanted to find out if you considered adding such support? This should also make it possible to turn the generators into Boost.Range ranges. 3. Regarding both generator and coroutine classes. Their nested self_t class should perhaps be called caller_t as it encapsulates the caller's context and allows yielding the value _to_ the caller. It might then be possible to overload operator()(...): void gen_evens(gen_t::caller_t& caller) { for( int i = 0; i < 10; i++ ) caller(i*2); } This will achieve a nice symmetry with code that is sending values to the coroutines with the use of operator(). Regards, -Eugene
Hi Eugene,
1. What is the rationale for having the generator-function's return statement be used to return the last generated value or forcing the use of yield_break (which uses exceptions that can be slow). My only experience with generators/coroutines has been in Python which might have biased my view but I think having the generator-function return void would be more natural (and would eliminate the need for yield_break). For example, if I wanted to generate a sequence of evens, I could write:
typedef boost::coro::generator< int > gen_t;
gen_t evens(int cnt) { return [=](gen_t::self_t& self) { for( int i = 0; i < cnt; i++ ) self.yield(i*2); }; }
the generator<> template was derived from coroutine<> and therefore it generator follows the practice of coroutien return_type(). Of course I think it would be possible to require a generator-function of void() with self_t::operator( T t) (T = int, ... == return type from generator) - I should think about it (if and how it is implementable - what think the community members?). some compilers complain some compilers to no if you end a function without a return statement. yield_return() does complete the generator and unwind the stack (AFAIK python coroutines are stackles). Unforutnately each compiler has its own unwind facility so it is easier to throw an exception in order to unwind the corutine/generator.
2. Considering that generators are used to produce a sequence of values, would it make sense to support range based for loop? While playing with the library, I defined a (hacky) iterator together with begin/end functions that made it possible to:
for( int x: evens(10) ) std::cout << x << ' ';
Of course iterators should benefit from coroutines/generators in some places. Your code looks like C# - I assume evens(10) is the range enumerator based on a generator?
I found such usage very natural and wanted to find out if you considered adding such support? This should also make it possible to turn the generators into Boost.Range ranges.
I agree.
3. Regarding both generator and coroutine classes. Their nested self_t class should perhaps be called caller_t as it encapsulates the caller's context and allows yielding the value _to_ the caller. It might then be possible to overload operator()(...):
void gen_evens(gen_t::caller_t& caller) { for( int i = 0; i < 10; i++ ) caller(i*2); }
This will achieve a nice symmetry with code that is sending values to the coroutines with the use of operator().
maybe - I used Giovanni Derettas interface design. At least you need to call an equivalent to yield_break() - terminating the coroutine - too. THis might break the symmetry. regards, Oliver
On Tue, Sep 4, 2012 at 1:38 AM, Oliver Kowalke <oliver.kowalke@gmx.de> wrote:
typedef boost::coro::generator< int > gen_t;
gen_t evens(int cnt) { return [=](gen_t::self_t& self) { for( int i = 0; i < cnt; i++ ) self.yield(i*2); }; }
the generator<> template was derived from coroutine<> and therefore it generator follows the practice of coroutien return_type(). Of course I think it would be possible to require a generator-function of void() with self_t::operator( T t) (T = int, ... == return type from generator) - I should think about it (if and how it is implementable - what think the community members?).
some compilers complain some compilers to no if you end a function without a return statement.
Do you mean in the cases where the function has been declared to return something other than void? Because I'm proposing to have generator-function's return type be void -- there should be no problems in that case.
yield_return() does complete the generator and unwind the stack (AFAIK python coroutines are stackles). Unforutnately each compiler has its own unwind facility so it is easier to throw an exception in order to unwind the corutine/generator.
You mean yield_break, right? I would keep yield_break as is for the cases where one wants to terminate the generator from a call to another function. e.g. void foo(gen_t::self_t& self) { ... self.yield_break(); } void gen_func(gen_t::self_t& self) { foo(self); ... } But I would think that many generator-functions would exit naturally from top-level and saving yield_break in that case would improve style and performance.
for( int x: evens(10) ) std::cout << x << ' ';
Of course iterators should benefit from coroutines/generators in some places. Your code looks like C# - I assume evens(10) is the range enumerator based on a generator?
It might look like C# :) evens() is the function defined question (1) and I got it to work with based-range-for by defining this hackish iterator: template <typename G> struct gen_iter { G* g; typedef decltype(std::declval<G>()()) result_type; result_type operator*() const { return (*g)(); } gen_iter& operator++() { return *this; } bool operator!=(const gen_iter&) const { return *g; } }; namespace boost { namespace coro { template <typename G> gen_iter<G> begin(G& g) { return gen_iter<G>{&g}; } template <typename G> gen_iter<G> end(G& g) { return gen_iter<G>{nullptr}; } } }
Am 04.09.2012 20:56, schrieb Eugene Yakubovich:
some compilers complain some compilers to no if you end a function without a return statement. Do you mean in the cases where the function has been declared to return something other than void? yes Because I'm proposing to have generator-function's return type be void -- there should be no
On Tue, Sep 4, 2012 at 1:38 AM, Oliver Kowalke <oliver.kowalke@gmx.de> wrote: problems in that case. yes - If the other reviewers agree I refactor generator<> so that the generator functions are required to return void and generator< T ::self_t::yield( T t).
yield_return() does complete the generator and unwind the stack (AFAIK python coroutines are stackles). Unforutnately each compiler has its own unwind facility so it is easier to throw an exception in order to unwind the corutine/generator. You mean yield_break, right?
yes - I mea yield_break() (AFAIK yield_return was a C# keyword)
I would keep yield_break as is for the cases where one wants to terminate the generator from a call to another function. e.g.
void foo(gen_t::self_t& self) { ... self.yield_break(); }
void gen_func(gen_t::self_t& self) { foo(self); ... } OK But I would think that many generator-functions would exit naturally from top-level and saving yield_break in that case would improve style and performance.
for( int x: evens(10) ) std::cout << x << ' '; Of course iterators should benefit from coroutines/generators in some places. Your code looks like C# - I assume evens(10) is the range enumerator based on a generator? It might look like C# :)
evens() is the function defined question (1) and I got it to work with based-range-for by defining this hackish iterator:
template <typename G> struct gen_iter { G* g; typedef decltype(std::declval<G>()()) result_type;
result_type operator*() const { return (*g)(); } gen_iter& operator++() { return *this; } bool operator!=(const gen_iter&) const { return *g; } };
namespace boost { namespace coro { template <typename G> gen_iter<G> begin(G& g) { return gen_iter<G>{&g}; } template <typename G> gen_iter<G> end(G& g) { return gen_iter<G>{nullptr}; } } }
seams interesting regards, Oliver
Hello Eugene, Am 04.09.2012 01:30, schrieb Eugene Yakubovich:
1. What is the rationale for having the generator-function's return statement be used to return the last generated value or forcing the use of yield_break (which uses exceptions that can be slow). My only experience with generators/coroutines has been in Python which might have biased my view but I think having the generator-function return void would be more natural (and would eliminate the need for yield_break). For example, if I wanted to generate a sequence of evens, I could write:
typedef boost::coro::generator< int > gen_t;
gen_t evens(int cnt) { return [=](gen_t::self_t& self) { for( int i = 0; i < cnt; i++ ) self.yield(i*2); }; } I managed to implement your suggestion - generator-functions are required to return void and generator<>::self_t::yield() accepts the return type specified as template arg of generator<>. generator<>::self_t::yield_break() was removed.
The code is available under git://gitorious.org/boost-dev/boost-dev.git (branch coroutine). regards, Oliver
Hi Oliver,
I managed to implement your suggestion - generator-functions are required to return void and generator<>::self_t::yield() accepts the return type specified as template arg of generator<>. generator<>::self_t::yield_break() was removed.
The code is available under git://gitorious.org/boost-dev/boost-dev.git (branch coroutine).
Thank you for implementing the changes. I know I initially argued to eliminate yield_break but later agreed that it was probably beneficial to have it for the cases when one wants to complete the generator from non top-level function. With the exception machinery there anyway, I think it won't hurt to keep it. Another issue that I see is with reference/pointer types. The fact that the following code will blow up is troubling: struct X {...}; typedef boost::coro::generator<X*> gen_t; void foo(gen_t::self_t& self) { X x1, x2; self.yield(&x1); self.yield(&x2); } gen_t g(foo); while( g ) { X* val = g(); // use *val. Ok on first iteration, dangling pointer on the second!!! } Especially if generator does not support movable-only types, I am afraid that this will be a common tripping point. Giovanni's design had the same issue which he advised to avoid by using the iterator like interface (using operator* and operator++). Maybe we can change behavior so that generator's constructor does not invoke the generator-function, only operator() does. And change operator() to return optional<R> instead of R. The usage can then be: gen_t g(foo); while( optional<X> val = g() ) { // use val.get() } I'm not really sure about what to do about is_complete() or operator unspecified-bool-type(), though. Maybe they are not needed at all? I admit that this interface might be a bit quirky but if the library also provides input iterator, that can become the primary interface. Generators are used to produce sequences and iterators are the central mechanism of dealing with sequences in C++. So the iterator usage could be: for( gen_t::iterator it = g.begin(); it != g.end(); ++it ) { X* val = *it; // no dangling pointers } (or BOOST_FOREACH, C++11 range-based for, or for_each). g.begin() or gen_t::iterator ctor would invoke the generator-function for the first time and ++it for all subsequent times. An alternative approach could be to try to make generator itself into an input iterator (like Giovanni's design) but that is hard because generator is non-copyable. Thoughts? Regards, -Eugene
Another issue that I see is with reference/pointer types. The fact that the following code will blow up is troubling:
struct X {...}; typedef boost::coro::generator<X*> gen_t;
void foo(gen_t::self_t& self) { X x1, x2; self.yield(&x1); self.yield(&x2); }
gen_t g(foo); while( g ) { X* val = g(); // use *val. Ok on first iteration, dangling pointer on the second!!! }
X * val is an address of x1/x2 on the stack of foo(). no danglingpointer until coroutine is alive.
An alternative approach could be to try to make generator itself into an input iterator (like Giovanni's design) but that is hard because generator is non-copyable.
I've to think about it. Oliver
Another issue that I see is with reference/pointer types. The fact that the following code will blow up is troubling:
[snip]
X * val is an address of x1/x2 on the stack of foo(). no danglingpointer until coroutine is alive.
Yes, the address would still be valid (stack is not gone) but the object would have already been destroyed. Maybe a better example: struct X { ~X() { std::cout << "~X" << std::endl; } }; typedef boost::coro::generator< X* > gen_t; void foo(gen_t::self_t& self) { X x1, x2; self.yield(&x1); self.yield(&x2); } int main() { gen_t g(foo); while( g ) { X* val = g(); std::cout << "use val" << std::endl; } return 0; } Outputs (ran against the coroutine branch): use val ~X ~X use val Clearly the use of x2 occurs after its destruction.
Am 11.09.2012 17:05, schrieb Eugene Yakubovich:
X * val is an address of x1/x2 on the stack of foo(). no danglingpointer until coroutine is alive.
Yes, the address would still be valid (stack is not gone) but the object would have already been destroyed. Maybe a better example:
struct X { ~X() { std::cout << "~X" << std::endl; } };
typedef boost::coro::generator< X* > gen_t;
void foo(gen_t::self_t& self) { X x1, x2; self.yield(&x1); self.yield(&x2); }
int main() { gen_t g(foo); while( g ) { X* val = g(); std::cout << "use val" << std::endl; }
return 0; }
Outputs (ran against the coroutine branch):
use val ~X ~X use val
Well - what should I do to prevent this? I think I can't. This is an issue not only related to boost.coroutine you can always construct such failures with pointers/references. (remember my example of dereferencing an reference, stored as member in a class.) Oliver
Especially if generator does not support movable-only types, I am afraid that this will be a common tripping point. Giovanni's design had the same issue which he advised to avoid by using the iterator like interface (using operator* and operator++).
It is an issue of boost.tuple - if tuples can handle moveable objects, boost.coroutine will handle it too.
Maybe we can change behavior so that generator's constructor does not invoke the generator-function, only operator() does. And change operator() to return optional<R> instead of R. The usage can then be:
gen_t g(foo); while( optional<X> val = g() ) { // use val.get() }
what would be the benefit?
I'm not really sure about what to do about is_complete() or operator unspecified-bool-type(), though. Maybe they are not needed at all?
coroutines are used like this: - 'if ( coro)' // coroutine has an coroutine-fn attached and can be used - 'coro.is_complete()' coroutine has returned and can not resumed generators: - 'if (gen) // generator is valid and has an value so that 'gen()' returns a value generators have no 'is_complete()' function (contained in the unspec_bool operator()).
I admit that this interface might be a bit quirky but if the library also provides input iterator, that can become the primary interface. Generators are used to produce sequences and iterators are the central mechanism of dealing with sequences in C++. So the iterator usage could be:
for( gen_t::iterator it = g.begin(); it != g.end(); ++it ) { X* val = *it; // no dangling pointers }
(or BOOST_FOREACH, C++11 range-based for, or for_each).
g.begin() or gen_t::iterator ctor would invoke the generator-function for the first time and ++it for all subsequent times.
Shouldn't iterators be implemented on top of generators/coroutines? Oliver
Maybe we can change behavior so that generator's constructor does not invoke the generator-function, only operator() does. And change operator() to return optional<R> instead of R. The usage can then be:
gen_t g(foo); while( optional<X> val = g() ) { // use val.get() }
what would be the benefit?
This would eliminate the "early destruction" problem described before. The generator would end up not storing a value (which it currently does in detail::generator_resume::result_) and operator() would just return whatever the generator-function yielded or optional<none> if the generator-function exited.
Shouldn't iterators be implemented on top of generators/coroutines?
Yes, that is definitely a good option. My thinking of putting the iterator "inside" the generator was to potentially avoid constructing optional<R> -- which would otherwise be needed if the generator's interface was like the one proposed above. Iterator could then use private methods of generator and not the public interface. I'm not sure if there's really any overhead to constructing optional<R> so it might not really matter. Also, looks like Boost.Optional does not yet support moveable-only types so having the iterators not use it can bring quicker support of movable-only types (after tuples support them). Granted, only the iterator interface would be usable with movable-only types. Oliver, if you would like, I can code up my proposed changes and send in the patch. It might make it easier to explore the design. Regards, Eugene
Am 11.09.2012 17:43, schrieb Eugene Yakubovich:
Maybe we can change behavior so that generator's constructor does not invoke the generator-function, only operator() does. And change operator() to return optional<R> instead of R. The usage can then be:
gen_t g(foo); while( optional<X> val = g() ) { // use val.get() } what would be the benefit?
This would eliminate the "early destruction" problem described before. The generator would end up not storing a value (which it currently does in detail::generator_resume::result_) and operator() would just return whatever the generator-function yielded or optional<none> if the generator-function exited. OK - maybe I should implement it and test the timings of both versions
Shouldn't iterators be implemented on top of generators/coroutines?
Yes, that is definitely a good option. My thinking of putting the iterator "inside" the generator was to potentially avoid constructing optional<R> -- which would otherwise be needed if the generator's interface was like the one proposed above. Iterator could then use private methods of generator and not the public interface. I'm not sure if there's really any overhead to constructing optional<R> so it might not really matter.
Also, looks like Boost.Optional does not yet support moveable-only types so having the iterators not use it can bring quicker support of movable-only types (after tuples support them). Granted, only the iterator interface would be usable with movable-only types.
Oliver, if you would like, I can code up my proposed changes and send in the patch. It might make it easier to explore the design.
OK regards, Oliver
Sorry I missed the official review period, I just wanted to pipe up to say that I've been severely missing the lack of a "generator" functionality in C++ for almost two decades, having become very accustomed to it in CLU (the language that inspired the "yield" concept for both C# and Python) where they were called "iterators." So I'm very excited to see it become part of the boost library and expect to heavily use it. I think generators can revolutionize the way C++ programmers iterate over collections, complex structures like trees, and potentially infinite sequences by making such abstractions very easy to write and easy to use. I built and played with this library a bit using Microsoft Visual Studio 2010. My thoughts so far: - I like the idea of incorporating Eugene Yakubovich's adapter to allow use of generators in range-based for loops (though I didn't try it since VS 2010 doesn't support those yet.) Being able to call generators with a syntax like "for( int x: evens(10))" or "BOOST_FOREACH( int x, evens(10) )" will make them ridiculously easy to use. I also like the suggestion of not requiring a return statement or yield_break() to end the generator. - Changing the interface to return boost::optional as was suggested has a potential minor drawback in terms of performance, but it creates a very clean and simple single-call interface, provided it's made clear that first time the optional returns nothing, iteration is complete. Also I'd hope that this does not conflict with supporting the above range-base-for syntax. - In the first example in the coroutine documentation ctx.is_complete() is called but ctx is not defined - I believe that should be c.is_complete()? Thank you and I enthusiastically look forward to using boost generators. -- View this message in context: http://boost.2283326.n4.nabble.com/Review-Coroutine-reviewstarts-today-Septe... Sent from the Boost - Dev mailing list archive at Nabble.com.
Here are my answers the specific review comments.
Please always state in your review, whether you think the library should be accepted as a Boost library! Yes!
- What is your evaluation of the design? I only evaluated the use of the generator class. Generator has a very nice interface, especially as improved during the course of this review.
- What is your evaluation of the implementation? I did not evaluate the implementation.
- What is your evaluation of the documentation? Fine. For illustrating the capabilities and simplicity of generators I might suggest a generator example that potentially generates an infinite sequence (e.g. primes(), where the caller breaks out of the loop when it finds a large enough prime), and/or an example of iterating over all nodes in a tree structure.
- What is your evaluation of the potential usefulness of the library? I personally think the generator class has the potential to revolutionize the way C++ programmers think about abstracting iteration. However, I've been a fan of the concept for decades so that doesn't mean the concept will necessarily catch on.
- Did you try to use the library? With what compiler? Did you have any problems? Compiled with Microsoft Visual Studio 2010 and it worked fine.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I spent about two hours building the examples and trying a few of my own.
- Are you knowledgeable about the problem domain? I have experience using generators in other languages; mostly in CLU years ago, a little in Python and C#. I use Boost regularly but among this group I'm a Boost novice.
-- View this message in context: http://boost.2283326.n4.nabble.com/Review-Coroutine-reviewstarts-today-Septe... Sent from the Boost - Dev mailing list archive at Nabble.com.
Am 11.09.2012 17:43, schrieb Eugene Yakubovich:
Maybe we can change behavior so that generator's constructor does not invoke the generator-function, only operator() does. And change operator() to return optional<R> instead of R. The usage can then be:
gen_t g(foo); while( optional<X> val = g() ) { // use val.get() } what would be the benefit?
This would eliminate the "early destruction" problem described before. The generator would end up not storing a value (which it currently does in detail::generator_resume::result_) and operator() would just return whatever the generator-function yielded or optional<none> if the generator-function exited.
changed in git-repo regards, Oliver
Hi all, This is my first review for Boost, since I am not a great C++ expert. Nevertheless, Boost.Coroutine addresses a problem that I was recently facing so I could form a valid opinion. The problem that I was facing was the need to interrupt, and later resume, the dijkstra_shortest_path algorithm. Following the documentation for Boost.Coroutine is was straightforward to rewrite the original function to a coroutine, and the result works just as it should with minimal debugging and no hidden surprises. To me this means that the library is extraordinary user-friendly, and it gives me control over my algorithms in a way that I did not have before. I therefore vote *FOR* acceptance. On 03/09/2012 12:46, Hartmut Kaiser wrote:
- What is your evaluation of the design? Good. It is very easy to use. - What is your evaluation of the implementation? Can't tell. - What is your evaluation of the documentation? Looks good. However, for a casual user like me it seems more relevant to understand the control flow of the coroutines than details about stack space and exceptions. All the 'Notes' and 'Warnings' are very prominent at the cost of the main points to be understood.
- What is your evaluation of the potential usefulness of the library? Good - Did you try to use the library? With what compiler? Did you have any problems? I used it with VS2010, without any problems. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I just used the library to solve my particular case, and read the documentation for as far as necessary. I did not look at the implementation at all. I did not investigate the performance, but did verify the results. My experiment is at http://people.pwf.cam.ac.uk/ahh34/dijkstra.zip
- Are you knowledgeable about the problem domain? No. Regards Hartmut Review Manager Kind regards, Alex
Hi, as there was some requests by some reviewers I've a modified version available at git://gitorious.org/boost-dev/boost-dev.git (branch coroutine). changes: - generator function must return void and generator<>::self_t::yield_break() is removed - StackAllocator is type-erased in coroutine/generator and can be passed in the corresponding ctor of both templates - an allocator can be specified to the ctor of coroutine/generator in order to customize the allocation of inner structures (documentation contains not all suggested changes yet) regards, Oliver
On Sun, Sep 9, 2012 at 3:23 AM, Oliver Kowalke <oliver.kowalke@gmx.de>wrote:
Hi,
as there was some requests by some reviewers I've a modified version available at git://gitorious.org/boost-dev/**boost-dev.git<http://gitorious.org/boost-dev/boost-dev.git>(branch coroutine).
changes: - generator function must return void and generator<>::self_t::yield_**break() is removed - StackAllocator is type-erased in coroutine/generator and can be passed in the corresponding ctor of both templates - an allocator can be specified to the ctor of coroutine/generator in order to customize the allocation of inner structures
I've been more or less following the discussion, and these all look like positive changes.
(documentation contains not all suggested changes yet)
Are you planning on updating the documentation in the near future (i.e., before the end of the review [September 12th I suppose])? - Jeff
Am 09.09.2012 17:03, schrieb Jeffrey Lee Hellrung, Jr.:
On Sun, Sep 9, 2012 at 3:23 AM, Oliver Kowalke <oliver.kowalke@gmx.de>wrote:
Hi,
as there was some requests by some reviewers I've a modified version available at git://gitorious.org/boost-dev/**boost-dev.git<http://gitorious.org/boost-dev/boost-dev.git>(branch coroutine).
changes: - generator function must return void and generator<>::self_t::yield_**break() is removed - StackAllocator is type-erased in coroutine/generator and can be passed in the corresponding ctor of both templates - an allocator can be specified to the ctor of coroutine/generator in order to customize the allocation of inner structures
I've been more or less following the discussion, and these all look like positive changes.
(documentation contains not all suggested changes yet)
Are you planning on updating the documentation in the near future (i.e., before the end of the review [September 12th I suppose])?
yes, I hope I get it ready before the review ends regards, Oliver
Am 09.09.2012 17:03, schrieb Jeffrey Lee Hellrung, Jr.:
Are you planning on updating the documentation in the near future (i.e., before the end of the review [September 12th I suppose])?
updated in git-repo (docu for this version at : http://olk.bplaced.net/boost/libs/coroutine/doc/html/) Oliver
Oliver, have you considered a solution where it is possible to inherit from a base class, so we can use the this pointer instead of the self parameter?
Am 09.09.2012 17:54, schrieb Bjorn Reese:
Oliver, have you considered a solution where it is possible to inherit from a base class, so we can use the this pointer instead of the self parameter?
Yes - but I decided to re-use Giovanni's interface design. Additionally, if you derive from a base class you get access to coroutine's internals which is not desired.
The review of Oliver Kowalke's Boost.Coroutine library starts today, September 3rd 2012, and will end on September 12th. I really hope to see your vote and your participation in the discussions on the Boost mailing lists!
Please always state in your review, whether you think the library should be accepted as a Boost library!
YES, I think it should be accepted.
Additionally please consider giving feedback on the following general topics:
- What is your evaluation of the design?
Overall, I like the design. coroutine class design is very good. As others have pointed out, it has some sharp edges (e.g. accessing coroutine-function arguments after first yield can cause access to bad memory) but I think that it is not a showstopper. I view coroutine class as a more advanced usage so some caution will be required. I would like to see symmetric coroutines in the future, though. I spent most of the time studying generator class. I think generators are easier to grasp and thus will be used more often (especially by novices). Like outlined in the previous posts, I would like to have generator-functions return void (Oliver already implemented this on a branch); have iterator support for use with algorithms and BOOST_FOREACH/C++11 range-based for; and address the danger associated with using pointer/reference types (either change the design or clearly document them). I also agree that having support for memory allocator (beside stack allocator) is a good idea and that they should both be type erased.
- What is your evaluation of the implementation? I did not study it in great detail but what I saw was of very good quality.
- What is your evaluation of the documentation? Good. There are some mistakes but others have already pointed them out.
- What is your evaluation of the potential usefulness of the library? Extremely useful. Should make event based applications easier to write. Will greatly simplify writing stateful iterators.
- Did you try to use the library? With what compiler? Did you have any problems? Yes. I used GCC 4.6.3 with no problems. I tried Clang 3.1 but it fails to compile -- simply including boost/coroutine/all.hpp generates errors.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I spent a few hours studying the library, experimenting, reading background info (e.g. Giovanni's original documentation).
- Are you knowledgeable about the problem domain? Only in that I have used generators/coroutines in Python for a number of years. I have also written many event based applications in C++ and can appreciate the need for coroutines in that domain.
Regards, Eugene
participants (7)
-
Alex Hagen-Zanker -
Bjorn Reese -
Conrad Poelman -
Eugene Yakubovich -
Hartmut Kaiser -
Jeffrey Lee Hellrung, Jr. -
Oliver Kowalke