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

Subject: Re: [boost] [review][coroutine] A Late Review
From: Oliver Kowalke (oliver.kowalke_at_[hidden])
Date: 2012-09-17 03:03:59

> I dislike the namespace.  "coro" is meaningless and awkward.

STL's namespace is 'std' I tought it's ok if I do it liekwise.

> 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.

The base class asserts (BOOST_ASSERT) pre-conditions in its functions like resume()/suspend(). In non-debug builds (optimization doesn't matter) the assertions remain until compiler flag BOOST_DISABLE_ASSERTS is applied.

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

OK, thx for the hint.

> 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?

sub-directory contains 'example' contains code demonstrating how to use a coroutine together with an streambuf in order to provide an non-blocking istream. The stream reads data from a socket demultiplexed by boost::asio::io_service. Might this be 'non-trivial'?

I think the documentation should describe simple examples and the 'example' sub-directory contains more complex ones (but harder to describe).

> 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.

After looking over the suggested modifications It seams that not very much of Giovanni's design will remain (self_t will be removed from coroutine-fn signature).

> 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.

It should, if you do not follow the recommendations in boost.exception the exceptions thrown inside the coroutine-fn will be re-thrown with type 'unknown_exception'.

> I suppose that isn't true in C++11.

I don't know. At least for compilers no implementing C++11 it should be true.

> 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.

'forced_unwind' is in 'detail' namespace. The user should not use it because the exception is used to unwind the stack.

> 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.

OK

> Also, the note, in those sections, references "ABIs", but gives no 
> references and fails to specify which ABIs are meant.

Should I not expect that the user knows what an ABI is and which ABI is used on its platform?
Should I add descriptions of all available ABIs?
 

regards,
Oliver