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

Subject: Re: [boost] [review][coroutine] A Late Review
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2012-09-17 08:23:27

Oliver Kowalke wrote:
>
> > I dislike the namespace.  "coro" is meaningless and
> > awkward.
>
> STL's namespace is 'std' I tought it's ok if I do it
> liekwise.

Should your library be standardized, it may appear in the std namespace or some subnamespace which could well be std::coro.  However, Boost has policies on namespaces and "coro" doesn't fit.

> > 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 didn't even look under libs to see the examples until you prompted me.  However, I don't see that example.  Nevertheless, it sounds interesting.

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

All of the examples, or most if there are a great many, should be described in the documentation, with links to the full source.  This is true for two reasons.  First, those reading the documentation are not looking through the source tree.  Second, the documentation affords the opportunity to discuss the examples freely, giving an introductory explanation as well as dissecting and discussing interesting parts of each.

As there are no comments in the examples, documentation is even more important.  Even if you added comments, which is reasonable, the examples should be introduced, at least, in the documentation.

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

Your mention of his design should be moved to the Acknowledgements section, then.

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

You need to be specific about those points, then.

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

If you missed it, I'm referring to std::exception_ptr, std::current_exception(), and std::rethrow_exception().

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

This is confusing then.  You reference the exception type by name, illustrate catching it by name, as well as having a catch-all handler, thereby suggesting that users can write that same set of handlers, but the type name is in the detail namespace, so users shouldn't reference it.

If users shouldn't know the name, then you shouldn't mention it.  Instead, you should tell them they mustn't absorb unknown exceptions because they'll absorb an important exception used by the coroutine implementation.  Thus, you should illustrate what's acceptable by:

try
{
   // coroutine-using code
}
catch (user-known-exception-type const & e)
{
   // rethrow or absorb e
}
catch (...)
{
   // react to exception
   throw; // rarely good to absorb unknowns
}

If you intend that they should be able to differentiate between your exception and others, then the name shouldn't be in the detail namespace and you should show the following:

try
{
   // coroutine-using code
}
catch (boost::coroutines::forced_unwind const &)
{
   throw;
}
catch (user-known-exception-type const & e)
{
   // rethrow or absorb e
}
catch (...)
{
   // react to exception
   throw; // rarely good to absorb unknowns
}

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

I didn't mean to suggest you needed to define ABI, though a link to Wikipedia or similar location wouldn't hurt.  Instead, my concern is that you refer to "ABIs" as if to say all extant ABIs.  I doubt you've studied them all, so that's too broad.  Perhaps you mean, "the ABIs on all supported platforms".

> Should I add descriptions of all available ABIs?

No, but links to details wouldn't hurt if practicable.

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