[boost] Re: Formal Review: Circular Buffer

From: Jan Langer (jan_at_[hidden])
Date: 2004-03-08 15:30:44

Hi,
I did a short review of this library.

- regarding the data() issue I think it should be mentioned that this 
function has linear complexity. A name reflecting this behavior would be 
even better.

- the difference between resize() and set_capacity() should be mentioned 
more explicitly. It might be especially confusing since std::vector's 
reserve is quite similar to set_capacity but has a different name.

- I am missing the counterpart to erase which shifts the elements before 
the erased elements and not the ones after. Probably this is not 
important but std::deque allows erasing at the front without 
invalidating all iterators (according to sgi-stl docs).

- The insert() function has its rinsert() counterpart. The same issue 
for resize and set_capacity is solved with the boolean parameter 
remove_front. If this has an important reason it should be mentioned. If 
not I would like to see a more consistent solution.

- In section Caveats:
"... According to the semantics of rinsert, insertion overwrites 
front-most items as necessary ..." should probably read "back-most 
items" if this is a valid english word.

- I compiled the library with g++ 3.2.2 and everything worked fine. But 
when I tried to compile the example from the documentation with a 
command line call to g++ I got the following error:

[jan_at_affe test]$ g++ -Wall -Wno-long-long -ansi -pedantic -I .. -I 
$BOOST_ROOT cb_example.cpp
../boost/circular_buffer/base.hpp: In member function `void
    boost::circular_buffer<T, Alloc>::replace(Alloc::pointer,
    boost::call_traits<Alloc::value_type>::param_type) [with T = int, 
Alloc =
    std::allocator<int>]':
../boost/circular_buffer/base.hpp:1137:   instantiated from `void 
boost::circular_buffer<T, 
Alloc>::replace_last(boost::call_traits<Alloc::value_type>::param_type) 
[with T = int, Alloc = std::allocator<int>]'
../boost/circular_buffer/base.hpp:605:   instantiated from `void 
boost::circular_buffer<T, 
Alloc>::push_back(boost::call_traits<Alloc::value_type>::param_type) 
[with T = int, Alloc = std::allocator<int>]'
cb_example.cpp:10:   instantiated from here
../boost/circular_buffer/base.hpp:1105: invalid use of member `
    boost::cb_details::cb_replace_category_traits<int>::tag'

Without -ansi -pedantic it works fine. I don't know if it should work 
with -ansi -pedantic but I thought it worth mentioning

- In general the documentation is quite good and well written.

- I didn't look at the code in detail, but in order to understand some 
things I had a look at certain member functions and found them clearly 
implemented and easy to understand.

- I cannot sufficiently evaluate the usefullness of this library since I 
haven't had the need for it in my previous work in C++. Actually I 
cannot imagine it being usefull in normal programming tasks where memory 
is plenty. And  I haven't done embedded programming in C++ yet.

- I spent approximatly 3 to 4 hours on studying the docs, clarifying 
certain things with the help of the code and compiling and looking at 
the examples and writing this email.

- I'm no expert in the problem domain. Just a regular user of std:: and 
other containers.

If some points mentioned above will be clarified and if other people (or 
me in the future) can find applications for it, I think its a very 
usefull library.
Since I am quite sure that this will be the case I vote for acceptance.

regards
jan

-- 
jan langer ... jan_at_[hidden]
"pi ist genau drei"