Apologies to Hartmut, Oliver, and the community for my late presence regarding the recently reviewed Coroutine library. I finally had a good block of time this afternoon/evening to look through the documentation [1], so I figured, even if Hartmut doesn't count this as a formal review, at least Oliver can use these comments to improve the documentation. First, if I were to vote on whether Coroutine should be accepted, it would be CONDITIONAL ACCEPTANCE just based on present documentation issues. At a minimum, the documentation needs some fixing up, but I also think Oliver should address the design and interface suggestions Vicente, Eugene, and others (I myself haven't yet gone through the entire discussion yet, though I've been meaning to) have had, which mostly seem to have merit, and then present an updated. Unfortunately, I can't come up with a specific list of said suggestions, but I trust Oliver can pick through the discussion as well as I can :/ Second, I would really like to see some form of this library in Boost :) One thing that benefits the documentation of a library with a seemingly nontrivial design space (like Coroutine) is a comparison with past and present solutions. A quick google search of "c++ coroutine" found 3 libraries that ostensibly implement coroutines or an approximation thereof [2-4]. I haven't gone through these quite yet, but I'd like to see why one should use the proposed Coroutine library over these alternatives (even a 1-line justification, like "Library X lacks critical feature Y, by design" or "Library X does not work on platform Z" would be sufficient.) I think some documented research to justify the existence of this library is warranted. My specific comments on the documentation follow (I just copy/pasted from a Google document, so I hope this doesn't format badly...). I'm pretty sure some of what's below repeats what someone else has said, but, for completeness, I just included everything that initially came to mind as I was reading the documentation. - Rename namespace boost::coro to (e.g.) boost::coroutine or something similarly spelled out and not (arguably cryptically) abbreviated. - "Executing a coroutine" - s/it can migrated/it can be migrated/ - The first example uses “coro_t c( boost::bind( f, _1 ) );”; I’m confused as f is a binary function, but the boost::bind expression only works for unary functions. - “The coroutine-function, as well as its arguments, if any, are copied into the coroutine's state.” Can you elaborate on what you mean by “its arguments...are copied into the coroutine’s state”? You must mean when you call the coroutine, i.e., the copying of the coroutine-function and the copying of the coroutine-function’s arguments occur at different times, right? - The example with output uses ctx when (I believe) you mean c. Also, it implies that coroutine<>::is_complete actually executes the coroutine-function, which seems non-intuitive to me; I would expect it to be non-modifying and only indicate whether the coroutine-function has completed or not. - s/preemtive/preemptive - “Calling coroutine<>::operator()() from inside the same coroutine results in undefined behaviour.” Can you give an example of how one could possibly mistakenly do this? - s/allways/always - “The arguments passed to coroutine<>::operator()(), in one coroutine, is returned by coroutine<>::self_t::yield() in the other coroutine or, if it is the first call to coroutine<>::operator()() ( coroutine was not started), the coroutine-function will be entered and the arguments are passed to coroutine-function on entry.” I think I know what you’re trying to say here, but talking about 2 coroutines when your examples only have one coroutine object is ripe for confusion. Either clarify your terminology, clarify this explanation of the signature, or both. - The fact that self_t::yield() must package its return value into a boost::tuple (when the coroutine-function has more than one argument beyond self) suggests to me that we should strive for more uniformity between how the coroutine-function is initially entered (now with explicitly separate arguments) and subsequently continued (now with a packaged set of arguments). A couple of suggestions: - Force all coroutine-functions to be nullary or unary (sans the self argument); one can manually address more than one argument via boost::tuple packaging, and maybe boost::coro::coroutine could automatically package >1 argument up into a boost::tuple to convenience. You may also want to provide a macro (named, e.g., BOOST_TUPLE_UNPACK) that can can unpack a tuple into variables, e.g., something that looks like BOOST_TUPLE_UNPACK( ( int x ) ( int y ), tuple ); - Use a coroutine-function signature that looks something like void f(coro_t::self_t & self) { BOOST_TUPLE_UNPACK( ( int x0 ) ( int y0 ), self.arguments() ); // … BOOST_TUPLE_UNPACK( ( int x1 ) ( int y1 ), self.yield() ); } Perhaps equivalently, one can always call self.arguments() to get the most recently passed set of arguments. - “coroutine-function with multiple arguments” - Isn’t the boost::bind in this example superfluous? - “Exit a coroutine-function” - yield_break or yield_return? You mention both. - There are 2 “Important” notes on forced_unwind exceptions that appear to say the same thing. - “Stack unwinding” - I’m starting to get really confused by your uses of boost::bind; “boost::bind( fn, _1 )” when fn is a nullary function??? I guess you forgot the self_t argument? If so, again, why is boost::bind even necessary? Did you check that these examples actually compile? - The default behavior of the destructor of always unwinding the stack seems entirely reasonable, but the preconditions you list seem entirely unreasonable and unsafe! Basically, I’m reading that { coro::coroutine< void ( ) > c; } results in undefined behavior, as the preconditions for the destructor are not met. This almost has to be a documentation error. Further: “After unwinding, a coroutine is complete.” I was under the impression that unwinding only occurs in the destructor of the coroutine, at which point the coroutine object does not exist anymore, so what coroutine are you referring to, and even if you’re referring to the recently destructed coroutine, in what way is it complete? - “Executing a generator” - s/The default StackAllocator concept/The default StackAllocator/ - Be consistent on when you use StackAllocator, stack-allocator, etc. I.e., what do you mean by each? - “The first argument of generator-function...”: You mean the *only* argument? - s/The generator-function executed/The generator-function is executed/ - Same notes as at the bottom of “Executing a coroutine”. - “Exit a generator-function” - Again, yield_break or yield_return? - “boost::bind( fn, _1, _2 )” ??? Huh? How is “a” bound within fn, then??? - s/self_yield/self.yield/ - It’s unclear to me why a separate generator class is provided in addition to the coroutine class. Can you explain what the differences are? E.g., I don’t see why coroutine can’t also have an operator unspecified-bool-type () rather than is_complete. - “Performance” - 10 doesn’t seem like a very convincing sample size. If you can’t nail down the CPU cycles and time exactly via theoretical analysis of the assembly code, then I don’t see what’s preventing you from using a sample size several orders of magnitude larger than 10! Okay, now time to go through the second half of the recent Coroutine discussion... - Jeff [1] http://ok73.ok.funpic.de/boost/libs/coroutine/doc/html/index.html [2] http://code.mozy.com/projects/mordor/ [3] http://www.akira.ruc.dk/~keld/research/COROUTINE/ [4] http://dunkels.com/adam/pt/