Re: [boost] [Review:Algorithms] - Small review

Subject: Re: [boost] [Review:Algorithms] - Small review
From: Marshall Clow (mclow.lists_at_[hidden])
Date: 2011-09-26 17:19:11

On Sep 26, 2011, at 1:36 PM, Christian Holmquist wrote:

> Hi Marshall and others,
> 
> I vote Yes to accept the library.

Thank you!

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

Since neither of them have a current maintainer, it would make sense.

I will take a look at them, and see what it will take.
        https://github.com/mclow/Boost.Algorithm/issues/13

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

Hrmmm...

> Do you intend to maintain the library at git?

I do not.
My expectation is that once the library is accepted, then I will check it into subversion, and further development will happen there.

However, I can see using the git repo for the development of new algorithms, ones that have not been reviewed yet.

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

I'm feeling for some kind of organization that will work as the library grows - for both the code and the documentation.
Suggestions welcome.

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

Already noted:
        https://github.com/mclow/Boost.Algorithm/issues/10

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

OK.

> - General -
> Is it necessary to have both Range and free Iterator support?

That seems to be what other libraries in boost have done.
It does increase the number of functions, though.

> all.hpp use #pragma mark, probably not portable. (indeed, MSVC 2005
> complains..)

I will be taking them out:
        https://github.com/mclow/Boost.Algorithm/issues/6

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

OK.

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

Fixed - and pushed to github.

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

Yes, it would.

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

These will be going away.

> 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

I'll take your word that this will fix 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

This is worrysome; thanks for opening a github ticket!

-- Marshall

Marshall Clow     Idio Software   <mailto:mclow.lists_at_[hidden]>

A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
        -- Yu Suzuki