On 10 Nov 2014 at 0:39, Edward Diener wrote:
- What is your evaluation of the design?
It's okay. I would prefer a design where there is a sorted_view which if std::copy gives an actually sorted copy. Obviously one would then specialise that scenario with a more directly optimised specialisation, and you'd also have the API presented here which looks like std::sort(). I'd also prefer to see fallback implementations for less than random iterators, right down to forward only iterators.
- What is your evaluation of the implementation?
There seems to be an assumption that user supplied overloads and specialisations e.g. swap() don't throw exceptions. I think that unwise. BOOST_NOEXCEPT can test if something can throw. Explicit guarantees of what happens when something does throw needs to be in the documentation, per API. There seems to be a general lack of explicit C++ 11 support as well. In particular, I think all Boost libraries should use inline namespaces to implement ABI version management. Implementation isn't capable of dual build as standalone use without any dependency on Boost. I'd suggest my forthcoming Boost.BindLib library.
- What is your evaluation of the documentation?
Lacks exception guarantees, one per API. Lacks scaling graphs near front so people can quickly evaluate usefulness compared to std::sort. I believe the author will fix this though.
- What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems?
No.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Glance.
- Are you knowledgeable about the problem domain?
Author of a very popular open source indexing algorithm library.
And finally, every review should attempt to answer this question:
- Do you think the library should be accepted as a Boost library?
Yes with these conditions: 1. Either change its name to Boost.Spreadsort, or do a much better job at a general sort library e.g. sorted_view, more than one sort algorithm not in the STL, more than random access iterator support. 2. Exception safety is not obvious, and explicit exception guarantees need to be made and stated per API in the documentation. 3. Explicit ABI version management in C++ 11 using inline namespaces. Forthcoming Boost.BindLib can do this for you, or it can be done by hand. It would also be nice if this library were also capable of standalone build without Boost, but this is not a requirement. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/