Re: [boost] Re: Formal Review: Circular Buffer

From: Jan Gaspar (jano_gaspar_at_[hidden])
Date: 2004-03-05 04:27:03

Hi Thorsten!

--- Thorsten Ottosen <nesotto_at_[hidden]> wrote:
> Hi Jan,
> 
> This is not my final review, but just some comment
> and questions.
> It's based solely on my reading of the docs.
> 
> 1. your rationale mentions many characteristics that
> I would expect of
> std::deque. Do you have any idea
>    how big the performance difference is?
The memory is allocated at once, that's the biggest
difference.

> 
> 2. You mention "Guarantee of basic exception safety"
> as a design criteria. I
> would expect many operations
>     to have a stronger guarantee.
Of cource, some methods provide stronger guarantee,
but in general the container povides just basic
exception safety. For some methods (e.g. insert) it is
impossible to provide stronger guarantees.

> 
> 3. you write "In fact the circular_buffer is defined
> in the file
> boost/circular_buffer/base.hpp, but it is necessary
> to ". I don't see
>    why the user should be bothered with this
> implementation detail.
You're right.

> 
> 4. Type Requirements: don't T need to be Assignable
> too?
Just no!

> 
> 5. you could remove one version of push_back and
> push_front by using default
> arguments. Similar for insert()/rinsert()
I don't know if this is good idea or not, but all STL
implementations I've seen have containers with both
methods (one with default param and one without).

> 
> 6. What's the use of "return_value_type"
> ?return_value_type  operator[]
> (size_type index) const
>         Return the element at the index position.
>   Should it not be const_reference?
return_value_type is just optimization. It is
const_reference for objects and value_type for
primitive types.

> 
> 7. when you state contracts like
> Precondition:
> *(this).size() > index
> don't you mean (*this).size() ? I would prefer that
> you omitted this
> entirely.
This is just typo. Anyway I would retain the
precondition.

> 
> 8. maybe you should add const_pointer data() const ?
It is not possible. data() is mutating operation. See
the source code.

> 
> 9 I assume you provide the basic exception safety
> for set_capacity().
> However, I think you could achieve the strong
> guarantee
>    by (1) allocating the new buffer and (2) start to
> copy elements [might
> throw] (3) swap the two buffers upon completion. The
> same goes for
>    resize(). Why are they both provided? (If you
> made resizes 2nd argument
> to be the bool, then how would they differ?]
Yes, it works exactly like this. But the statement
about provided exception safety covers the whole
container.

> 
> 10.  Can't the exception guarantee for the
> copy-constructor be stronger too?
> (and assign(), and push_back(), push_front(),
> insert() )
No for insert()/rinsert(), push_xxx(), data() and
erase(). See the source code.

> 
> 11. Some operations must be nothrow: swap(), size(),
> capaciity(),
> pop_back(), pop_front().
Yes, they are. It is not written explicitly in the
documentation (there is just no "Exceptions" section
for each method).

Jan

__________________________________
Do you Yahoo!?
Yahoo! Search - Find what you’re looking for faster
http://search.yahoo.com