Re: [boost] [Boost-users] Formal Review: Boost.RangeEx

Subject: Re: [boost] [Boost-users] Formal Review: Boost.RangeEx
From: Christopher Jefferson (chris_at_[hidden])
Date: 2009-02-21 06:50:30

On 20 Feb 2009, at 12:28, Thorsten Ottosen wrote:

> Dear Developers and Users,
>
> It's my pleasure to announce that the review of Neil Groves' RangeEx  
> library starts today and lasts until March 3, 2009.
>
>
> Writing a review
> ++++++++++++++++
>
> If you feel this is an interesting library, then please submit your  
> review to the developer list (preferably), or to the review manager.

The following review is based on reading the documentation, looking  
through the source, and mapping some code using an existing custom  
range library to this code.
> Here are some questions you might want to answer in your review:
>
> - What is your evaluation of the design?

The library seems to provide a firm foundation to an important area.

>
> - What is your evaluation of the implementation?

>

The code seems very clean and well designed, there do not seem to be  
any surprises.

> - What is your evaluation of the documentation?
>

The documentation is simple, but mostly complete. A few small comments:
1) I assume the intention is that most functions delegate to the  
library implementations (for example range sort -> std::sort).  
Explicitly stating if this is, or is not, the case would be useful.
2) overwrite's documentation could do with being longer (but I will  
comment on this further later).
3) The return types of prev_permutation and next_permutation are wrong  
(should be bool, not void)

> - What is your evaluation of the potential usefulness of the library?

Initially very useful at just making functions smaller and neater, I  
believe the Range Adaptors (which I have not used) will provide  
further power later.
>
> - Did you try to use the library? With what compiler? Did you have  
> any problems?

I used g++ 4.3 without problems.

There are a few areas I would like to see fixed, these are very minor  
changes.

1) There seems some unnecessary inconsistency in return values, for  
example generate returns the range, but fill returns void.
2) I would like to see partial_sort return the partially sorted range.
3) The *_heap functions should also return the original range.  
Certainly sort_heap should for consistency with sort.
4) I have found it useful to have a simple helper function that turns  
a single value into a range with one value.

The larger area I would really like to see improvement in is copy /  
overwrite. I believe some bigger improvements, particularly in the  
area of safety, have been missed here.

1) overwrite should state what happens if the output range is  
overflowed. I would like an exception to be thrown.
2) Why isn't overwrite just an overload of copy?
3) copy_backward should also have a two-range overload.

The obvious problem with bounds checking is efficiency. However from  
my experience, it is easy to check random access ranges by a couple of  
calls to 'distance', and for bidirectional and forward ranges, the  
overhead of checking for end of range is very small. If it isn't  
acceptable to adding checking to copy, I would like to see a  
checked_copy.

I am happy to contribute code to this area.

>
> - How much effort did you put into your evaluation? A glance? A  
> quick - reading? In-depth study?

A quick reading, seeing if it implemented the features in our existing  
library.
>
> - Are you knowledgeable about the problem domain?

Reasonably.
>
>
> And finally, every review should answer this question:
>
> - Do you think the library should be accepted as a Boost library?
>

Yes