I like what I see so far, but I'm missing the coroutine future API. We use Giovanni Deretta's Summer of Code Coroutine prototype in production code, and we rely on his 'future' semantics. My intention was to test Oliver's Coroutine proposal by replacing it into our code, but this is not possible without something equivalent to 'future'. Accordingly, this review is based on the library documentation rather than on actual usage. (One could imagine passing a coroutine object itself as a callback for some other library, then calling yield() to await that callback. There are two obstacles. One is that I know of no way to obtain, within the coroutine-function, a reference to the containing coroutine object. The initial 'self' parameter is not that object; it presents a different API. A more serious problem is that the value returned by yield() is fixed by the coroutine template's Signature parameter. With futures, each future object can return a different type; a given coroutine can instantiate an arbitrary number of future objects with an arbitrary number of types to await an arbitrary sequence of callbacks.)
Please always state in your review, whether you think the library should be accepted as a Boost library!
I vote: conditionally accept. I cannot switch to this library without 'future' support.
Additionally please consider giving feedback on the following general topics:
- What is your evaluation of the design?
Good
- What is your evaluation of the implementation?
Did not review
- What is your evaluation of the documentation?
Generally good; detailed suggestions follow
- What is your evaluation of the potential usefulness of the library?
HIGH. More people need this functionality than realize it. :-)
- Did you try to use the library? With what compiler? Did you have any problems?
I did not
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I read the documentation at the cited link: http://ok73.ok.funpic.de/boost/libs/coroutine/doc/html/
- Are you knowledgeable about the problem domain?
Yes ======================================================================== Detailed comments on documentation: First example in section "Coroutine": "boost::coro::coroutine< void() > c( make_coroutine() );" This example might benefit from a couple comments explaining its purpose. As the first example in the documentation, I thought it might illustrate a short but complete use case. It's only intended to demonstrate a coroutine object being returned by value from a function; but when I first read it I thought make_coroutine() was part of the library API, like make_shared(). "The coroutine-function, as well as its arguments, if any, are copied into the coroutine's state. If a reference is required, use boost::ref." Excellent! This is an important point. "The maximum number of arguments of coroutine-function is 10." Is that limit adjustable with a #define symbol? If not, it should be. Second example under "Executing a coroutine": coro_t c( boost::bind( fn, _1, 7) ); std::cout << "main() starts coroutine c" << std::endl; // yield was called so we returned ^ This comment, at this point in the code, perplexed me. while ( ! ctx.is_complete() ) ^ 'ctx' undefined, I think you mean coro_t 'c' { std::cout << "main() calls coroutine c" << std::endl; // execution control is transfered to c c(); // yield() was called within fn() } "In contrast to threads, which are preemtive, coroutine switches are cooperative (programmer controls when a switch will happen). The kernel is not involved in the coroutine switches." s/preemtive/preemptive/ I would love it if, somewhere in the introductory material, the library documentation emphasized the pragmatic benefit of cooperative switching: it requires no new defensive locking. s/allways/always/ "The arguments passed to coroutine<>::operator()(), in one coroutine, is returned by coroutine<>::self_t::yield() in the other coroutine..." At first glance this makes the reader wonder how yield() can possibly return up to 10 coroutine arguments. May I suggest: "...is returned (as a boost::tuple) by..." ? Example under "coroutine-function with multiple arguments": "boost::tuple< int, int > ret = self.yield( tmp);" This describes exactly the type returned, so is correct. Even so, I find myself thinking that a dubious reader might be more favorably impressed by an example using a couple scalar variables with boost::tie(). And it might clarify the example to use different types for the coroutine parameters. Example under "Exit a coroutine-function": s/self.yield_return()/self.yield_break()/ "An exception thrown inside coroutine-function (transfered via exception-pointer - see Boost.Exception for details and requirements) will be re-thrown by coroutine<>::operator()()." I'm not familiar with Boost.Exception. Will any exception be re-thrown, including exceptions raised by the runtime? Or does this only apply to exceptions derived from a particular base class, or thrown in a particular way? If there are such limitations, mentioning them briefly here would be extremely useful. "Code executed by coroutine must not prevent the propagation of the forced_unwind exception. Absorbing that exception will cause stack unwinding to fail. Thus, any code that catches all exceptions must rethrow the pending exception." This and the example should clarify that we're talking about code within the coroutine-function. (And is forced_unwind also within the boost::coro namespace?) If forced_unwind is the exception thrown when a (! is_complete()) coroutine object is destroyed, that's worth mentioning. It's possible that a coroutine-function might want to perform cleanup specific to that case. coroutine::self_t reference: "T yield( R);" T and R undefined. "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() );" flag_unwind_t undefined. "R operator()(A0 a0, ..., A9 a9);" R, A0, ... A9 undefined. Example in section "Generator": Undefined make_generator() function wants a comment. Again, this is a hypothetical user function rather than part of the boost::coro::generator API. "The first argument of generator-function must be of type generator<>::self_t, ..." The "only" argument? I'd like to see some words when generator is first introduced clarifying the ways in which it differs from coroutine. This what I think: * A generator-function accepts no arguments (other than self_t), so yield() always returns void. * yield_break() does not throw an exception in the invoking coroutine. * Anything else? It might be worth a few words to clarify that when you pass a boost::bind() expression, *that* is the generator-function, and it must accept only self_t. Any other parameters bound by boost::bind() are irrelevant to boost::coro::generator. In that connection, the function f() in the first "Executing a generator" example should probably accept a different parameter type than int so the reader can clearly distinguish between the type accepted by f() and the type passed to yield(). And in *that* connection, the self.yield() call in that example should pass an int value. "The generator-function, as well as its arguments, if any, are copied into the generator's state." Um, what arguments? Again, if you're using boost::bind() to adapt a function that accepts anything more than self_t, processing of additional arguments is up to boost::bind() -- not boost::coro::generator. "The maximum number of arguments of generator-function is 10." What does this mean? "generator-function is invoked the first time inside the constructor of generator." That makes no sense to me. Is the value passed to the first yield() call simply discarded? If so, why? "If generator-function can not return valid values anymore generator<>::self_t::yield_break() should be called. This function returns the execution control back to the caller and sets the generator to be complete (is_complete() returns true)." is_complete() is not documented for boost::coro::generator. "Of curse the generator-function can be exited with a return expression." s/curse/course/ Is there any semantic difference between executing 'return' in a generator-function and calling yield_break() in a generator-function? If not, is yield_break() provided only for consistency with coroutine? If a generator-function has a declared return type, is that value discarded? Example in "Exit a generator-function": "self_yield( i);" should be self.yield(i); "self.yield_return();" should be self.yield_break(); class generator::self_t reference: "void yield( R);" Is R the same as Result here? "Result operator()() Preconditions: *this is not a not-a-generator, ! is_complete()." is_complete() is not documented for boost::coro::generator. "void self_t::yield( Result) Effects: Gives execution control back to calling context by returning a value of type Result. The return type of this function is a boost::tuple<> containing the arguments passed to generator<>::operator()()." That entire last sentence seems untrue. "void self_t::yield_break() Effects: Gives execution control back to calling context and sets the generator to be complete. generator<>::operator()() in the other generator throws exception coroutine_terminated." Last sentence above contradicts other generator documentation.