RE: [boost] Dynamic Bitset Formal Review June 8-17

From: Powell, Gary (powellg_at_[hidden])
Date: 2002-06-17 15:32:37

Under the wire review:

  First: Thanks Chuck and Jeremy for all your work.

  Vote to accept with caveats.

  It gets iterators when iterators become finer granularity. aka
Jeremy's iterator stuff is accepted.

Issues:
-------------
  All "std::Blah" should change to "::std::Blah" if you really mean use
the global "std" namespace.

-------------
  typedef std::size_t size_type;

should become

  typedef allocator::size_type size_type;

and
  Block *m_bits

should become
  typedef allocator::pointer block_type_pointer;

  block_type_pointer m_bits;

It has bugged me to no end that the rest of the standard containers
don't pay attention to the typedefs in their allcoators and I think its
high time that at least new ones do. Otherwise what is the point of
those typedefs! (BTW, It makes it really hard to use shared memory
pointers etc, unless you do use the allocator typedefs.)

----------

In your class reference,

  don't you have to declare the friendship like this?

  friend template<class B, class A> class dyn_bitset;

----------

  Shouldn't "any" use "true and "false" instead of "0" and "1" it does
return a "bool"

  Also should you test for the zero case? And shouldn't you use pointer
math instead of relying on "operator[]" ? (Some compilers are pretty
dumb about optimizing this.)

  if (!m_bits)
    return false;

  for (block_pointer_type p = m_bits;
       p != (m_bits + m_num_blocks);
       ++p)
  {
     if (*p)
        return true;
  }
  return false;

--------------
I too want "capacity" and "reserve" Sometimes I know how many bits I'm
going to use, so I might as well let the container in on it as well.

-------------

dyn_bitset:
  This name either needs to go to "dynamic_bitset" or some other more
descriptive name.

------------
Implementation of swap:

  how about doing the old "using ::std::swap;" and then calling "swap()"
trick. It the same problem we've always had and the least bad of the
fixes.

  Yours,
 -gary-