Re: [boost] [review][sort] Formal review period for Sort library begins today, November 10, and ends Wednesday, November 19

Subject: Re: [boost] [review][sort] Formal review period for Sort library begins today, November 10, and ends Wednesday, November 19
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2014-11-14 07:31:29

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/