On 06/01/2014 10:07 a.m., Nat Goodspeed wrote:> Hi all,
Please always state in your review whether you think the library should be accepted as a Boost library!
My vote is to REJECT the library in its current state.
Additionally please consider giving feedback on the following general topics:
- What is your evaluation of the design?
The design is that of the C++11 thread API, so I'll only focus in the divergence points: - The lack of a variadic constructor for `fiber` and variadic arguments for `async` makes it difficult to use the library correctly (even in the presence of C++14 lambdas). The semantic of those calls is that of a deferred call, which is difficult to achieve otherwise (note that `bind` doesn't help here). - The interface can only accept a specific clock `time_point`. Correct use of Boost.Chrono is needed, the implementation can deal with a specific clock type internally. - There is no support for deferred futures, which are incredibly useful for lazy evaluation. - There are a number of minor issues with the interface (return types, parameters). These are easily fixable by looking at the standard. - The safe-bool operator is a pointless divergence which only helps save a few keystrokes. I'm ok with them staying, but please prioritize the weak and missing points of the library first. The overall impression is that the library leaves the boilerplate to users (bundling a deferred call, converting to a specific clock). Also, it's crucial to get the semantics for `fiber` constructors right which means being careful about certain details (like making sure they work with movable-only types), but those constructors are not there yet.
- What is your evaluation of the implementation?
I've only glanced at the implementation, and I have concerns about the quality of the code. For instance, the following pattern appears frequently and it's unsettling: #ifndef BOOST_NO_RVALUE_REFERENCES promise( promise && other) /* bunch of code... */ #else promise( BOOST_RV_REF( promise) other) /* same code as above... */ #endif For completeness, the correct use of Boost.Move is: promise( BOOST_RV_REF( promise) other) /* code, just once... BOOST_RV_REF(promise) will be promise&& if there is rvalue-refs support */ Unfortunately I do not have enough time now to look into it in more detail, but incorrect use of Boost.Chrono and Boost.Move is not a promising start.
- What is your evaluation of the documentation?
The reference documentation looks OK. Some points are missing, but I've already raised those and Oliver agreed to take care of them.
- What is your evaluation of the potential usefulness of the library?
The potential usefulness of this library is huge. It goes beyond that of simply being an utility library for Boost.Asio. Fibers are a great replacement for threads for two key points: the ability to create thousands (or millions) of them, and performance (lighter than a thread). It was hinted that performance is not a goal of this library, as it is merely intended to provide a way to synchronize/coordinate coroutines with Boost.Asio. If that's the case, I'd suggest to move the library to the `asio` namespace and leave the `fiber` namespace open for a fiber library that targets a wider audience.
- 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've looked at the documentation, glanced over the implementation, and followed the debate on the mailing list.
- Are you knowledgeable about the problem domain?
I work in and with a fiber-based C++11 thread API implementation. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com