[boost] [Review:Algorithms] - Small review

Subject: [boost] [Review:Algorithms] - Small review
From: Christian Holmquist (c.holmquist_at_[hidden])
Date: 2011-09-26 16:36:06

Hi Marshall and others,

I vote Yes to accept the library.

Comments follows as I've encountered them in the documentation.

- Description and rationale-
"Boost.Algorithm is a collection of general purpose algorithms. While Boost
contains many libraries of data structures, there is no single library for
general purpose algorithms"

boost/algorithm has existed for a while, but only the algorithm/string is
properly documented (I think).
Will
boost/algorithm/minmax.hpp
boost/algorithm/minmax_element.hpp
be incorporated in the documentation?

- Future plans-
"My goal is to run regular algorithm reviews, similar to the Boost library
review process, but with smaller chunks of code."

Maybe a short summary on how a submission should work? Email subject tags
etc, and what is required by the programmer (docs, code and tests).
Since the library is at git (great!), maybe  proposals could be done as push
requests, or submitters share a link to their github repo or so.
Having even a rough outline on the process would be better than none at all
:)
Do you intend to maintain the library at git?

* I think the style linking to the algorithm sections from the main page is
not optimal (i.e. Header: 'all.hpp' ). Maybe
<Suggestion>
Algorithms
      All
      Clamp
      Ordered
      Search
</Suggestion>
will read easier.

- Reference section

* Complexity is missing on each algorithm's page.

* I would like if the page for each algorithm contains a small example like
in the SGI's STL docs. They can be very trivial, but to me they are always
reassuring that I've understood the docs correctly.
Example
assert(boost::clamp(0, 1, 2) == 1);

* Every algorithm in the ordered section, has their iterator typename
spelled FI. It would read easier as ForwardIterator. Dito for R -> Range.

- General -
Is it necessary to have both Range and free Iterator support?
all.hpp use #pragma mark, probably not portable. (indeed, MSVC 2005
complains..)
all.hpp could use std::begin() if available from std, unless there's a
specific reason to always use the Boost version (?)
search.hpp checks for a macro B_ALGO_DEBUG. It should read
BOOST_ALGORITHM_DEBUG or similar to avoid name clashes.

*** What is your evaluation of the design?
Straight forward. Only worries a bit on the Range vs Iterator interfaces.
It's probably impossible to satisfy everyone without providing both
interfaces as has been done though.

*** What is your evaluation of the implementation?
Looks good to me. Well commented code with easy to read style. I can't
comment on the correctness of the search.hpp algorithms though, but the
tests are not passing here.

*** What is your evaluation of the documentation?
Everything needed is in the package, but the examples needs to be linked
into the docs.

***  What is your evaluation of the potential usefulness of the library?
Immediately useful. good potential to evolve further.

* How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
About 2.5 hours reading the docs, reading the code, running some examples
and some of the tests.

* Are you knowledgeable about the problem domain?
Somewhat.

*** Did you try to use the library? With what compiler? Did you have any
problems?
Visual Studio 2005 with native std (Dinkumware), Windows XP 64, 32 bit
target, boost 1.46.

All examples starting with
int main ( int /*argc*/, char */*argv*/ [] ) {
gives a warning on this compiler.

search_example.cpp:
d:\code\tests\tests\clamp.cpp(23) : warning C4138: '*/' found outside of
comment
int main ( int /*argc*/, char */*argv*/ [] ) {
                                        ^--------------------- add space
here between * and / to make MSVC happy.

d:\code\boost.algorithm\boost\algorithm\search.hpp(238) : warning C4267:
'argument' : conversion from 'size_t' to 'int', possible loss of data
There are few similar warnings, the warning log is about 75 lines so would
be good to get rid of.

Running search_example.cpp in debug mode gives assertion failure in MSVC's
Debug iterator code.
I've not investigated further,

all_example.cpp
d:\code\tests\tests\clamp.cpp(23) : warning C4138: '*/' found outside of
comment
warning C4068: unknown pragma

clamp_example.cpp
error C2679: binary '<<' : no operator found which takes a right-hand
operand of type 'std::string' (or there is no acceptable conversion)
#include <string> fixes this

search_test1.cpp
Running 1 test case...
Pattern is 8, haysstack is 37 chars long;
unknown location(0): fatal error in "test_main_caller( argc, argv )":
c:\program files (x86)\microsoft visual studio 8\v
c\include\xstring(111) : Assertion failed: string iterator not
dereferencable

In Release mode, the output is
Running 1 test case...
Pattern is 8, haysstack is 37 chars long;
unknown location(0): fatal error in "test_main_caller( argc, argv )":
Invalid parameter detected by C runtime library
.\clamp.cpp(71): last checkpoint

*** 1 failure detected in test suite "Test Program"

d:\code\tests\tests\clamp.cpp(71): last checkpoint

*** 1 failure detected in test suite "Test Program"

All tests pass except those related to search.hpp.
I should point out that I just copied the source from each test into a toy
visual studio project, but I've not had any troubles with that before.

Sorry for a bit unorganized output of the tests, will take some time this
evening to sort it out.

Thanks to Marshall and Dave for the library and for managing the review

- Christian