Re: [boost] Re: Formal Review Minmax

From: Herve Bronnimann (hbr_at_[hidden])
Date: 2002-10-07 11:39:42

On Thu, Oct 03, 2002 at 08:44:22PM +0100, Craig Henderson wrote:
> Here is my review of the MinMax algorithm in the sequence_algo library.

Thanks for your very thorough review work, which is highly appreciated.
I especially appreciate the compiler outputs, since I have access to
only a few compilers myself.

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

Agreed. It'll be changed.

> int test_main( int argc, char * argv[] )
> I guess I am doing something wrong?

I wouldn't think so. I thought both declarations would be valid, but I'm
no expert and haven't read the standard carefully about those issues (I
assume test_main has the same requirements as main).

> 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) :-)

I guess you're referencing the fact that the documentation states
head-on <boost/minmax.hpp>, but that the code lives in
<boost/sequence_algo/minmax.hpp>. I don't if anoyone mentioned this, I
myself noticed it too late, but it's not a big deal in any way. For one
thing, I could add a dummy file boost/minmax.hpp that include
boost/sequence_algo/minmax.hpp, although I'd like to maintain the
one-file-definition-rule :)

As to the sequence_algo, I would very much like the minmax library to
have neighbours. Jeremy is way too busy to do anything with the
containers right now, but there is also combinatorial.hpp (Phil
Garofalo) and your lcs implementation (which I am looking at right now,
and will send comments privately). So by all means!

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

I'm aware of the problems with VC++6 and plan to stay away as much as
possible from that beast (especially in template code). But
thanks to you, and Gennadiy, I'll make the small changes to compile with
Visual C++ V6, and can now include in the documentation:

   VC++6 (once it's fixed)
   VC++7 .net
   Borland CPP v5
   g++ 2.95, 3.0, 3.1

I'll be happy to add Metrowerks or Comeau if someone tries it (I'm
currently purchasing Comeau as an investment).

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

Please don't be. I have access to VC6 but consider it a lost cause. In
any case, Gennadiy already provided a patch.

> So I moved on to Microsoft Visual C++.Net (version 7)
> Build and run, Ok.

Very good, I don't have access to .net yet.

> Next, I tried Borland's Free C++ compiler version 5.5.1
> The build succeeded with warnings (see below) and then executed successfully
> 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.

I'll correct the warnings, they don't seem very serious.

> And finally, g++ v3.1.1.20020718 (prerelease) on Cygwin, Build and run Ok.
 
I had tested g++ 3.0 and 3.1 myself on Linux. Everything OK there as well.

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

Very much so, thank you so much.
-- 
Herve'