[boost] [review][coroutine] A Late Review

Subject: [boost] [review][coroutine] A Late Review
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2012-09-16 19:38:05

Coroutine should be accepted into Boost conditionally.  My hesitation comes from the seemingly significant design and interface changes occurring during the review.  I'm unsure how the current state compares to the library I reviewed.  This leaves me wondering what is being accepted.

I don't think the functionality of the library is changing, so I'm confident in accepting it on that basis.  However, I think that a mini-review is needed to verify the finished design and documentation before final acceptance.

***** What is your evaluation of the design?

I dislike the namespace.  "coro" is meaningless and awkward.  The namespace name should be "coroutines" (since there is a class named "coroutine", it must be plural).  What users may choose as an alias is another matter.

I wonder whether assertions can be added to flag calls of operator() from within a coroutine or generator.  I know that's undefined behavior, but an assertion will catch the mistake in non-optimized builds.

***** What is your evaluation of the implementation?

The source has major whitespace issues.  It looks like a botched tab-to-spaces conversion.

The move assignment operators do a self-assignment check.  That is a "performance bug", as Howard Hinnant puts it.

There are no comments in the code.  This means Oliver's own maintenance will be harder, but it particularly means that it will be harder for anyone else to pick up the library's maintenance to help or replace Oliver in the future.

The code looks well factored, though I certainly didn't spend a long time studying it.

***** What is your evaluation of the documentation?

There is no Motivation section.  The documentation must catch the attention of the reader right on the first page.  Why should anyone care about this library?  What can it do for them?  Why should anyone spend time reading the rest of the documentation?

Overall, the documentation is a good introduction to the library.  The examples are appropriate for tutorial purposes, but there is a complete lack of non-trivial examples to illustrate how the library can be employed.  What can one do with this library that isn't possible otherwise?  What is better, according to any number of criteria like less code, faster, etc., using this library?

How much of the interface remains what was designed by Giovanni Piero Deretta?  Given all of the changes during the review, I wonder how much remains.

The "Executing a coroutine" section of coroutine.html could do with a sentence about the coroutine-function's signature as, say, a new, third sentence in the first paragraph: "The coroutine-function can take arguments and return values as discussed in the next section."

The "Exceptions in coroutine-function" section of coroutine.html, and the corresponding section of generator.html, suggests that exceptions must be thrown using Boost.Exception.  Is that the intent?  If so, you need to make that point clear.  I suppose that isn't true in C++11.  Also, what is the namespace of forced_unwind?  This section is the first mention of it, so one is left to wonder if it belongs to the library or not.

The "FPU preserving" sections of coroutine.html and generator.html imply that the library allows to prevent preserving the FPU registers, but gives no information about how that is done or where to read more about it.  Also, the note, in those sections, references "ABIs", but gives no references and fails to specify which ABIs are meant.

The coroutine and generator synopses, in coroutine/coroutine.html and generator/generator.html, are confusing.  The definition of self_t refers to undefined types T and R.

The synopsis of coroutine suggests that the type is not copyable

Some detailed comments are at the end of this message.

***** What is your evaluation of the potential usefulness of the library?

I think this library will be extremely useful.  I think it could have profound implications for the design of libraries that rely on multiple threads today.  Indeed, there is a use case in some current code in one of our applications that, I think, would be simplified yet improved by using this library.

- Did you try to use the library?  With what compiler?  Did you have any
problems?

Unfortunately, and despite the lateness of my review, I've had no time to try it.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I spent several hours reading the documentation, and a fraction of that looking over the code.

***** Are you knowledgeable about the problem domain?

No.

___________________
Documentation Comments

________________
coroutine/coroutine.html

The illustration of make_coroutine() is accompanied by no description, so f() should be omitted.  It adds nothing one cannot infer from the signature and raises questions about what c and, in turn, f() actually does.

In the first warning, s/coroutine/a coroutine/.  In the second, s/::FlsAlloc(), ::FlsFree()/::FlsAlloc() and ::FlsFree()/.

s/stack-allocator/stack allocator/

Odd internal spacing for parentheses: one space after the open parenthesis, and no space before the close parenthesis.

s/operator()()/operator()/

s/flags, and stack and instruction/flags, stack, and instruction/

s/value of local variable is preserved/value of local variable i is preserved/

"yield was called so we returned" is confusing.  The coroutine hasn't been called by that point.  That comment should probably be, "while the coroutine hasn't returned" (or "finished").

In Transfer of data, the last sentence of the second paragraph (after the note) is rather confusing and includes a grammatical error.  I suggest this instead:

   "The arguments passed to coroutine<>::operator(), in one
   coroutine, are passed to coroutine<>::self_t::yield() in the
   other coroutine, except for the first invocation of
   coroutine<>::operator(), which passes them to the coroutine-
   function, instead."

The example in Transfer of data could use with a few more comments.  Specifically, "int x = c(7)" could use the comment, "x is 7 because c(7) passes 7 to fn, where self.yield(i) passes i == 7 back", and "x = c(10)" could use the comment, "x is 10 because c(10) causes "self.yield(i)" to return 10 in fn(), so fn() returns j == 10".  While you're at it, s/j == 10 because c( 10) in main()/j == 10 because of c(10) in main()/.

The coroutine-function with multiple arguments example deserves some comments explaining why yield() gets one int argument and returns a two-int tuple and why the function returns one int, to tie those aspects into the Signature template argument.

The Exit a coroutine-function section discusses yield_break(), but the example calls yield_return().

In the Important note within Exit a coroutine-function, s/complete (can not resumed/complete and cannot be resumed/.

In Stack unwinding, s/(stack is unwound by default)/(the default)/.

In FPU preserving's note, s/ABIs the/ABIs, the/.

________________
coroutine/coroutine/coroutine.html

In the Effects of the template constructor, s/stack before destructing it/stack before releasing it/.

In the Effects of the function call operator, the wording can be improved:

   "Invokes the coroutine-function with the supplied arguments,
   returning the argument passed to self_t::yield()."

Likewise, the Throws clause:

   "Any exception emitted by the coroutine-function or
   coroutine_terminated if self_t::yield_break() is called."

The Effects of self_t::yield() can be improved:

   "Returns execution control to the calling context, arranging
   for the calling context's operator() to return r."

(You need to make the argument list "(R r)" for that description.)

Next, yield() needs a "Returns" clause:

"Returns:
"   Arguments passed to the calling context's operator()."

Change the Effects clause for yield_break() to:

   "Marks the coroutine complete and returns execution control
   back to the calling coroutine in such a way as to cause the
   calling coroutine's operator() to emit a coroutine_terminated
   exception."

______________
generator.html

s/yielding a values/yielding values/

s/and can be returned/, so they can be returned/

make_generator() is not introduced.

The illustration of make_generator() is accompanied by no description, so f() should be omitted.  It adds nothing one cannot infer from the signature and raises questions about what c and, in turn, f() actually does.  (If you choose to keep it, perhaps "g" is a better identifier for a generator.)

In the second note, s/::FlsAlloc(), ::FlsFree()/::FlsAlloc() and ::FlsFree()/.  Also, that same information is marked as a warning for coroutine.

In the warning, s/If generator/If a generator/.

There are no comments in most of the examples on this page.

The fifth paragraph of Executing a generator needs to be rewritten to fix mistakes and improve clarity:

   "A generator is usually called inside a loop.  The generator-
   function calls generator<>::self_t::yield() to produce a
   value.  When the generator is called a second or subsequent
   time, the generator-function resumes execution after the
   yield() call.  If a generator-function must be called no more,
   it can call generator<>::self_t::yield_break()."

The sixth paragraph also needs to be rewritten:

   "The generator-function is invoked the first time from the
   generator's constructor.  generator<>::operator() can be
   called while the generator is valid (while
   generator<>::operator unspecified_bool_type() returns true).
   When the generator-function cannot return more values, it can
   return a value or call generator<>::self_t::yield_break().
   The latter returns execution control back to the caller and
   marks the generator complete.  Note that
   generator<>::operator() does not thrown an exception, unlike
   a coroutine."

(It isn't clear whether returning from a generator-function marks a generator complete.)

In the last Executing a generator example, the "yield was called so we returned" comment within main() is a bit unclear.  I suggest, "calls fn()" on g's construction and "fn()called self.yield(), so control returned here" on "while (g)".

In the Important note within Exit a generator-function, s/complete (can not resumed/complete and cannot be resumed/.

_______________
generator/generator.html

The Effects clause of operator() would be better separated into an Effects and a Return clause:

"Effects:
"   Invokes the generator-function."
"Returns:
"   The value passed to self_t::yield() by the generator-function."

Likewise, for self_t::yield():

"Effects:
"   Returns execution control to the calling context such that
    generator<>::operator() returns r in the calling context.
"Returns:
"   The arguments passed to generator<>::operator() in the
"   calling context."

The Effects clause for self_t::yield_break() is incorrect.  I suggest the following:

  "Marks the generator complete and returns execution control to
   the calling context."

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer                     using std::disclaimer;
Dev Tools & Components
Susquehanna International Group, LLP  http://www.sig.com

________________________________

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.