Boost logo

Boost :

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.


> =====================================================
> 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

> =====================================================
> 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.


Boost list run by bdawes at, gregod at, cpdaniel at, john at