Re: [boost] Re: Formal Review Minmax

From: Herve Bronnimann (hbr_at_[hidden])
Date: 2002-10-02 12:15:58

On Wed, Oct 02, 2002 at 02:39:44AM -0400, Gennadiy Rozental wrote:
> My general opinion that the submission should be accepted. Though it may
> require some polishing.

Thanks.

> =====================================================
> Design/Interface
> 
> In general I agree with you algorithm design and proposed simple function
> interface. Though I am not sure how functions like
> last_min_first_max_element are useful.

Truthfully, neither am I. I was aiming at completeness, and also because
strictly speaking these functions cannot be computed from the other ones
(even when reversing the range to invert first/last).

> =====================================================
> Implementation
> 
> 1. if (comp(b,a)) return std::make_pair(b,a);
>    else return std::make_pair(a,b);
> 
> Why not: return comp(b,a) ? std::make_pair(b,a) : std::make_pair(a,b);
> If should be ok since your both arguments are of the same type.

I think it's equivalent from a C++ point-of-view. That is, the compiler
would generate the same code.

> 2. I agree with your point that no need to make an interface policy based.
> But this does not mean that we need to repeat  the implementation 8 times
> with only slight change. All algorithms first/last_max/min_element could be
> implemented in terms of one generic algorithm. The same could be done (but a
> little bit more difficult) with first/last_min_first/last_max_element. Look
> onto attached file. Second generic algorithms is not implemented for all
> cases. It almost of top of my head, but you should get the idea.

What you're proposing is tantamount to using policies internally, and
exporting the non-policy versions. I have nothing against it, I did it
my way for historical reasons and also because when using one algorithm
I would just copy/paste it rather than including the whole file. For a
library, makes no difference. And it may help a transition toward
providing policies explicitly. SO why not.

> 3. I implemented looping in last_min_last_max a bit differently. I believe
> it could save you at least one comparison and more elegant in general (IMO).

I'll look at that!

> =====================================================
> Code
> 
> 1. Statements like if( a ) b; better to write in a form:
> if( a )
>    b;
> Particulaly, it simplify debugging, cause you have separate line for
> breakpoint;

Point taken.

> 2. min_result = second, potential_min_result = first;
> I would prefer regular block with 2 statements.
> 
> 3. The best code marker for clarity is .. empty line.
> IMO you lack empty lines between statements.

I like compact code, when it's warranted. But I also like clarity.
If implementing one one algorithm with policies, I could afford lots of
empty lines. Otherwise (times 8, as you note) it gets longish.

> 4. template <class ForwardIter>
> I would prefer: template <typename ForwardIter>

All right.

> =====================================================
> Example
> 
> Well. This is not an example. It's huge and difficult to understand. This is
> the best way to scare first-timer. We need several trivial examples that
> show basic usage of the algorithms. Current example may become second test
> module.

Yes, you are right. I did put an example in the doc (a short one), and
the example in the libs/sequence_algo/example directory is really a
tester (except it takes a long time so I didn't want to make it a test
module). I'll add simpler examples for the introductory value, and a few
notes.

> =====================================================
> Testing
> You test does not compile with MSVC6.5. I made several changes to make it
> compile and pass. Problems arose with your using of reverse_iterator (I
> switched to use boost::reverse_iterator), our old enemy template<typename T>
> foo(), that incorrectly linked by MSVC and iterator_traits for pointer that
> are not specialized due to lack of partial specialization.

Thanks. At the time, I was not writing with boost::reverse_iterator,
because I wanted to give the code to non-boost users (students).
Obviously the boost version is better, and since we're talking about a
boost library, it'll be available. So we should use it.

> Also you should not be using old BOOST_TEST tool. You would be better
> off with BOOST_CHECK_EQUAL in most cases and BOOST_CHECK. Look onto
> attached file.  Also you should name the file minmax_test.cpp

All right. Very valuable comments from you, obviously (about testing).
I'm not a regular user, so it's a nice bootstrap you give me. I'll
follow up in the sandbox.

> I am attaching modified minimal.hpp and minimal.cpp. It compiles and passes
> all tests with MSVC6.5.

Great. I'll put them in the library with your stamp on it.
Many thanks for the thorough review and the contributions.
-- 
Herve'