Re: [boost] Re: circular_buffer ver. 3.3 [long]

From: Jan Gaspar (jga_at_[hidden])
Date: 2003-09-02 10:09:10

Hi Pavel!

Thank you very much for your comments. I agree with most of them. Thanks also for
the picture. Here are my notes to some of your comments.

>
> Few notes to latest source:
>
> 1. circular_buffer_adaptor.html: the link in
>
>    "The circular_buffer_space_optimized is defined in
>     the file boost/circular_buffer.hpp."
>
>    points to wrong source file.

I just want the user to include the circular_buffer.hpp regardless if she uses
either circular_buffer or its adaptor. Maybe I should write that the
circular_buffer is defined in circular_buffe.hpp but the link will point to
circular_buffer_base.hpp. Similarly for the circular_buffer_space_optimized.

>
> 5. Borland C++ Builder 6.4 doesn't allow to bring
>    operator[] via using.

>
> This is workaround in circular_buffer_adaptor.hpp:
>
> #include <boost/detail/workaround.hpp>
>
> ...
>
> #if BOOST_WORKAROUND(__BORLANDC__, BOOST_TESTED_AT(0x564) )
>     reference operator [] (difference_type n) { circular_buffer<T,
> Alloc>::operator[](n); }
>     const reference operator [] (difference_type n) const {
> circular_buffer<T, Alloc>::operator[](n); }
> #else
>     using circular_buffer<T, Alloc>::operator[];
> #endif
>

What about just overriding operator[] for all compilers? (With the note that some
compilers doesn't support using for operators.)

> Also the "<< 1" and ">> 1" can be in this case safely replaced by * 2 and /
> 2
> (it is unsigned).
>

What is wrong with "<<1" and ">>1". I think it is safe and it is more effective
for some compilers which don't optimize the *2 operation.

>
> Shrink algorithm can be:
> a. allow hysteresis when shrinking
>    if new_size < current_capacity / 3 then shrink_to(std::max(min_capacity,
> current_capacity / 2))

Why current_capacity / 3 ?

>
> The constructors and set_capacity() would need one more paramerer or
> overload,
> capacity() could return pair<>. clear() would need change.

Maybe new method can be introduced e.g. min_capacity(). The capacity method stays
unchanged.

>
>
> 8. circular_buffer_space_optimized<> constructor with circular_buffer<>
>    parameter can be provided, similarly operators <, >, ==, !=,
>    and possibly swap() (swaping only items).
>

No, I don't want to go this way. Suppose some new circular_buffer adaptor with new
features will be introduced. Then you will have to provide operators for the base
circular_buffer and the circular_buffer_space_optimized. If you add another
adaptor the number of functions will rise exponencially.

If you look e.g. on std::stack it also doesn't provide operators for std::deque.

>
>
> 9. circular_buffer_base.hpp and circular_buffer_adaptor.hpp:
>    instead of check
>
> #if !defined(BOOST_NO_FUNCTION_TEMPLATE_ORDERING) || defined(BOOST_MSVC)
>
> is enough to use
>
> #if !defined(BOOST_NO_FUNCTION_TEMPLATE_ORDERING)
>
> (it is defined for MSVC).

I don't know why, but this does not work. You have to include "||
defined(BOOST_MSVC)" too.

>
> 14. Btw, isn't cb_iterator::operator[]() added by mistake?
>     I have never seen such an operation for iterator.
>

No, iterators do have this operator.

>
>
> 16. RAII vs try blocks:
>
> > > 1. IMO the macro based exception handling isn't needed, it is better
> > >     to use RAII, like:
> > > ...
> > >
> > > can be replaced by:
> > >
> > > void set_capacity(size_type new_capacity) {
> > >   if (new_capacity == capacity()) return;
> > >   pointer buff = allocate(new_capacity);
> > >
> > >   struct deleter_t {
> > >     pointer data;
> > >    size_type capacity;
> > >     deleter_t(pointer p, size_type s) : data(p), capacity(s) {}
> > >     ~deleter_t() { deallocate(data, capacity); }
> > >   };
> > >   deleter_t guard(buff, new_capacity);
> > >
> > >   size_type new_size = new_capacity < size() ? new_capacity : size();
> > >   std::uninitialized_copy(end() - new_size, end(), buff);
> > >   destroy();
> > >   m_size = new_size;
> > >   m_buff = m_first = buff;
> > >   m_end = m_buff + new_capacity;
> > >    m_last = full() ? m_buff : m_buff + size();
> > >    guard.data = 0;
> > > }
> > >
> > I think this is not a good idea because
> > - it won't work -  the ~deleter_t() destructor does not have access to
> > the deallocate() method
> > - the original macros are more explanatory (it is easier to understand)
> > - every STL implementation is using such macros
> >
> It may be possible to pass member function pointer or make deleter friend.
>
> (Another possibility would be to use Alexandrescu's ScopeGuard.)
>
> There's one good reason to avoid catch(...): on Win32 it interferes with
> system exceptions handling. If someone uses __try/__catch/__finally,
> these won't be called after throw; and also it won't be possible
> to fix and continue system exception.
>
> If I remember right, Digital Unix compiler had similar option for signals.

My colleague told me, if such a system exception ocurrs the destructor of the
exception guard won't be called. Check this out please. He also told me that the
system exceptions have nothing to do with C++ exceptions except that catch(...)
catches them. Btw. I would wonder if all the STL implementations would be just
wrong.

>
> 24. Question: do you plan to add Mojo to circular_buffer?

I don't know yet. I have to admit that I read the article about the mojo technique
just briefly. I will let you know when I decide.

Jano

--
Jan Gaspar | jga_at_[hidden]
Whitestein Technologies | www.whitestein.com
Panenska 28 | SK-81103 Bratislava | Slovak Republic
Tel [phone] | Fax [phone]