[boost] Re: Formal Review Minmax

From: Alisdair Meredith (alisdair.meredith_at_[hidden])
Date: 2002-10-07 13:17:38

Thomas Witt wrote:

> The formal review for the minmax algorithm starts today 28 sep and ends 7 oct.
> 
> Herve Bronnimann is the library author and I am the review manager.  Minmax is
> supposed to be part of a full fledged boost algorithm library if such a thing
> will ever make it to existance. For the time being we decided to review
> algorithms one-by-one or in small chunks.
> 
> The review sources and documentation can be found here:
> 
> http://groups.yahoo.com/group/boost/files/minmax_element/minmax.zip
> 
> For guidelines on formal reviews look at:
> 
> http://www.boost.org/more/formal_review_process.htm.

I have not had a chance to run the code through my compiler, so my
comments are entirely based on the documentation, which I assume is
accurate.  [I will run up some test cases later tonight, but don't want
to miss the review deadline]

My first reaction was to compare the spec against the similar utilities
we use ourselves, to see how they differ.

The main difference is that we pass in the result type as an output
parameter.  This is useful for us because we also support larger
statical return objects containing the sum, mean and variance.  That
being beyond the scope of this library, the pair return makes sense.

We also suffer from drop-outs in our data, represented by invalid values
[such as NAN]  We automatically filter such values when parsing the
data.  Again, where we have done this intrusively in our algorithms it
would seem the filter_iterator_adaptor combined with minmax do a
superior job, another point to minmax!

I wondered what would happen if minmax were given an empty range, and
the documentation is no clearer.  It merely insists that the passed
range is valid as a precondition.  Would 'valid and non-empty' be better
phrasing, or is an empty range automatically be assumed invalid?  This
is not the case for any of the STL algorithms I can recall, and so worth
highlighting anyway.

I'm not sold on the first/last algorithms, and did not follow why the
policy based approach was rejected.  Rather than use a default policy,
minmax_element could be overloaded so that the extra versions [that are
non-standard anyway] use the policy and the default version carries
straight over to standardisation.

A final thought was that the pair might be more efficiently built using
call_traits, but that may be a premature optimisation.

This being my first review, and critiquing another's work without
putting up any of my own, I find it hard to say 'no' so do not oppose
the addition of minmax into boost.  However, I would rather see more
discussion about the first/last variants before voting a wholehearted
'yes'.

AlisdairM