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@gmail.com> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki