[Boost-users] [Review.Coroutine] Coroutine review comments

Subject: [Boost-users] [Review.Coroutine] Coroutine review comments
From: Nat Linden (nat_at_[hidden])
Date: 2012-09-07 17:35:05

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.