[boost] Re: Formal Review Minmax

From: Craig Henderson (cdm.henderson_at_[hidden])
Date: 2002-10-03 14:44:22

Here is my review of the MinMax algorithm in the sequence_algo library.

First off, I'd like to cast a positive vote for the inclusion of the
algorithms into the Boost sequence_algo library and to thank Herve for his
efforts.

I have reviewed the library without reading other peoples remarks, to avoid
any contamination of my views. As such, there may be conflicts or duplicates
with others, for which I apologise but I believe this gives an independent
review and the library author can then consolidate the opinions.

Design

=====

I like the rationale behind the algorithms. It is nice to be able to get
both the min and max values by making a single pass over a sequence with
only a small performance overhead over getting the individual min() or max()
value. The interface is small and clean. However, I'm not clear on how
useful last_min_first_max_element() is. That said, it is nice to be complete
with the algorithm set, and shouldn't remove functions just because a use
cannot be envisaged now.

Implementation

===========

I don't have too much to say about this. Each to his own when it comes to
style and layout of code, and I didn't see anything erroneous or dangerous !
If pushed, the only point I'd make is that multiple statements per line can
make it more difficult to step through the code - most debuggers single step
and set break points on a line boundary, unless you switch to disassembly.

Before I could compile the test on any of my compilers, I had to change the
test_main() signature from

int test_main(char argc, char** argv)

to

int test_main( int argc, char * argv[] )

I guess I am doing something wrong?

File Structure

==========

I appreciate that MinMax is the first submission into what will become
sequence_algo library, and as such the minmax.hpp header file lives in the
boost/sequence_algo directory. However, is there no intention to create a
top level header file for the library as a whole, or will all submissions be
in this structure? I ask the question partly from a personal interest as my
upcoming Longest Common Subsequence algorithm will go into the sequence_algo
library too (if it is accepted, of course) :-)

Documentation

===========

The documentation is good. It recognised the limitations of the STL min/max
implementation and provides reference to further discussion. I agree that
these problems are outside of the scope of the MinMax library, and do not
see the same limitations in this library as any case for concern. It will,
of course, be nice to update this library in line with changes to the
standard in the furture.

Compiler compatibility

================

There is no compiler compatibilty section in the docs, AFAICS. In the
Performance section, you describe you benchmark environment as g++ version
2.95.2 under Linux. Is this the only compiler that has been tested so far? I
have compiled the test on four compilers, as listed below:

My first attempt at compiling the code was with Microsoft Visual C++ V6 SP5.
This failed to build, starting with

...\include\utility(81) : error C2039: 'iterator_category' : is not a member
of '`global namespace''

...\minmax.cpp(141) : see reference to class template instantiation
'std::iterator_traits<class custom *>' being compiled

...\minmax.cpp(160) : see reference to function template instantiation 'void
__cdecl test_range(class custom *,class custom *,int,char *)' being compiled

A total of 9 errors occured during the build.

Unfortunately, I only have access to VC6 at work where I do not have time to
spend on a full evaluation, so I cannot provide a patch. I'm sorry about
this.

So I moved on to Microsoft Visual C++.Net (version 7)

Build and run, Ok.

Next, I tried Borland's Free C++ compiler version 5.5.1

The build succeeded with warnings (see below) and then executed successfully

Borland C++ 5.5.1 for Win32 Copyright (c) 1993, 2000 Borland

minmax.cpp:

Warning W8004 ../../../../Library/Boost/Current\boost/test/test_main.cpp 61:
'result' is assigned a value that is never used in function
cpp_main(int,char * *)

Warning W8057 minmax.cpp 150: Parameter 'name' is never used in function
test_range<int *>(int *,int *,int,char *)

Warning W8008 C:\Program Files\Borland\BCC55\Include\vector.h 213: Condition
is always false in function __init_aux<int *>(int *,int
*,_RW_is_not_integer)

Warning W8066 C:\Program Files\Borland\BCC55\Include\vector.h 214:
Unreachable code in function __init_aux<int *>(int *,int
*,_RW_is_not_integer)

Warning W8057 minmax.cpp 150: Parameter 'name' is never used in function
test_range<custom *>(custom *,custom *,int,char *)

Warning W8008 C:\Program Files\Borland\BCC55\Include\vector.h 213: Condition
is always false in function __init_aux<custom *>(custom *,custom
*,_RW_is_not_integer)

Warning W8066 C:\Program Files\Borland\BCC55\Include\vector.h 214:
Unreachable code in function __init_aux<custom *>(custom *,custom
*,_RW_is_not_integer)

Turbo Incremental Link 5.00 Copyright (c) 1997, 2000 Borland

It would be nice if we could get a clean compile, if the warning in the
Borland STL are caused by something in the Boost code. I confess not to be
any kind of Borland expert. I currently only use this compiler for
cross-compilation testing, and no more.

And finally, g++ v3.1.1.20020718 (prerelease) on Cygwin, Build and run Ok.

I hope this information is useful to other Boost Developer list readers and
to Herve.

Regards,

-- Craig