RE: [boost] small but handy (to me) data buffer class

From: Matthew Hurd (matt_at_[hidden])
Date: 2004-03-27 04:44:35

Hi Rob,

Thanks your effort here, much appreciated.

> > > Isn't that the purpose of shared_ptr and shared_array?
> >
> > A shared_ptr idiom works just dandy.  Trouble is you have to add all the
> > alternatives with a handle-like idiom, or some such, which drives you to
> > policies and you end up with something similar in complexity anyway but
> > perhaps not quite as to the point.
> 
> Seeing the examples and code, I see that you're doing more than
> just managing the deletion of such buffers; you providing the
> means of allocation, the size, the data type, etc.

Yep, why waste seconds with struct buf { size_t size; char * data; }
when you can spend an order of magnitude longer making it pretty ;-)
 
> > Being able to annotate quite clearly and simply the appropriate
> > semantics
> > for data block ownership for network libs drove my turning
> > 	struct thing { size_t size; char * data; }
> > into several more lines.  Win32/64, bsd, ACE, Ensemble, etc all have
> > varying
> > semantics for handling data blocks which at least with simple policies
> > you
> > can approach declaratively.  At least that was the idea, not sure it is
> > not
> > overkill though as it is all much ado about nothing.
> 
> The policy based approach is quite flexible.  Too flexible I
> think.  See below.

I'll summarize here.  I think you're right and wrong.  Mostly right.

I think you're right about pairing the allocation and removal sides.  

If I include your comment later of:

> Combining your create_refer with kill_nothing would be more
> meaningful than either separately.  Can you think of an occasion
> when you'd want to allocate a buffer but not release it?

The answer to this is, "I often do this".  I use libraries where I must
allocate and then forget about the buffer and I use libraries where I must
accept a buffer and destroy.  I also use stuff where I must not delete the
data and I simply want to refer to an existing buffer.  I've done this with
math/statistic, messaging and process group libraries recently.

This means I want to optionally use the create and kill functions from the
storage policies.  I could do this with 
   1.  bools as template parameters, 
   2.  constructor values with even run-time modifiable state, or,
   3.  via another policy-like thing.

1. is not preferred by me as have true, false or true, true in a declarative
thing is confusing to me.  Could use macros or enums or static constants to
get around this I guess, but people will find a way around it ;-)

2. need to store the vars in the structure violating my need for simple
binary compatibility with popular buffer structs.

3. The only one left is a policy-like thing, this could be two, one for
allocation and one for deletion, like the flag thing, or just bung it into
one.  One will do for me.

What about?

struct storage_kill_only
{
    BOOST_STATIC_CONSTANT(bool, kill = true );
    BOOST_STATIC_CONSTANT(bool, allocate = false );
};

struct storage_allocate_only
{
    BOOST_STATIC_CONSTANT(bool, kill = false );
    BOOST_STATIC_CONSTANT(bool, allocate = true );
};

struct storage_no_allocation_or_kill 
{
    BOOST_STATIC_CONSTANT(bool, kill = false );
    BOOST_STATIC_CONSTANT(bool, allocate = false );
};

struct storage_allocate_and_kill
{
    BOOST_STATIC_CONSTANT(bool, kill = true );
    BOOST_STATIC_CONSTANT(bool, allocate = true );
};

> This is where I disagree with the flexibility.  I think the
> allocation and deallocation routines should be paired in a single
> policy.  That way, to create a new combination, one must do so
> expressly, not accidentally as the current design allows.

Paired is good, with the caveat that either one can be disabled.

 
> Why restrict MaxSize to be of type int?  You can eliminate
> SizeType; see below.

Yep, even eliminated it further.  How about

template < class T, std::size_t MaxSize  >
struct storage_policy_new
{
    static T * create()
    {
        return new T[MaxSize];
    }

    static void kill( T * p )
    {
        delete [] p;
    }
};

> Why not make this a function template based upon SizeType?  The
> compiler can even deduce the type needed and the policy user
> won't need to specify a type.

Good idea.  However, I've removed the need by centralising the memcpy into
the main class after thinking about one of your comments below.

> >     {
> >         return static_cast<T*>(memcpy( create(), source,  size *
> > sizeof(T) ));
> 
> Um, what if size > MaxSize?

Done.  Though I used assert.  Still not convinced BOOST_ASSERT is really
necessary.

> Same problem here.

Similar different solution:

template < class T, std::size_t MaxSize >
struct storage_policy_malloc
{
    static T * create()
    {
        return static_cast<T*>(malloc(MaxSize * sizeof(T) ));
    }

    static void kill( T * p )
    {
        free(p);
    }
};

> >     }
> > protected:
> >     ~create_malloc() {}
> > };
> >
> > // already allocated just refer to the space
> > template < int MaxSize, class T, typename SizeType  >
> > struct create_refer
> 
> "create_refer" didn't carry any meaning for me. "create_nothing"
> works a little better.  Still, it's a strange "creation" policy
> and helps to make my "combine them" case.  (See below.)

No need for this class with the separation of whether or not to allocate
from the storage policy.  However, it does feel a little strange doing a 

   data<2048,storage_policy_new, storage_no_allocation_or_kill>

So I added a 

template < class T, std::size_t MaxSize >
struct storage_policy_null
{
    static T * create()
    {
        return NULL;
    }

    static void kill( T * p )
    {
        
    }
};

which is superfluous but it is perhaps nicer to look at 

   data<2048,storage_policy_null, storage_no_allocation_or_kill>

rather than be forced to an arbitrary storage policy.

> > {
> >     static T * create()
> >     {
> >         // shouldn't be called
> >         BOOST_STATIC_ASSERT(false);
> 
> That will cause a compile-time assertion whenever this class is
> instantiated, won't it?  The only way that would be safe is if
> create() were a function template that was only instantiated if
> used.  To prevent calling this function, why not omit it?

Only if the default constructor is used, which it shouldn't have been, so it
could have been removed, but the assert message was cleaner to me.

Anyway, no need now.

> > // data owns the block, allocated with new T[]
> 
> It's comments like that that make me want to combine the
> policies.

Too true. Thanks.

> So here's my take:
> 
> template < typename T, typename MaxSize >
> struct storage_array_new
> {
>    static T * create()
>    {
>       return new T[MaxSize];
>    }
> 
>    template < typename SizeType >
>    static T * create(T * source, SizeType size)
>    {
>       BOOST_ASSERT(size <= MaxSize);
>       return static_cast<T*>(memcpy(create(), source,  size * sizeof(T)));
>    }
> 
>    static void kill(T * p)
>    {
>        delete [] p;
>    }
> 
> protected:
>    ~storage_array_new() {}
> };

A good guide thanks.  Though I've think my newer one is a little simpler.

<snip>
> Combining your create_refer with kill_nothing would be more
> meaningful than either separately.  Can you think of an occasion
> when you'd want to allocate a buffer but not release it?

Yes.  As discussed previously.  It is the interaction with third party
libraries with their own differing rules.

> Oops!  You used size_t (std::size_t, I assume) for MaxSize, but
> int for the corresponding argument in the creation policy
> classes.  I also don't think a default size is appropriate.
> There are way too many scenarios in which that default is wrong
> and too few in which it is right.

Thanks.  All std::size_t now, I hope.
Default size removed but I still have a data_standard typedef ;-)

> >     , template <int, class, class > class CreatePolicy = create_new
> >     , template <class> class KillPolicy = kill_delete
> >     , typename Base = char
>                  ^^^^
> Bad choice of name.  This is the data type, so "T" is customary
> and "Base" connotes base class.

Yep.  T it is.  

Also I think data is probably a bad name, depends on the namespace though.
net::data works for my network lib, but perhaps raw_buffer is more generic.

> >     , typename SizeType = size_t
> > >
> > class data : public CreatePolicy< MaxSize, Base, SizeType >, KillPolicy<
>                ^^^^^^
> Why?  Do you mean for clients to be able to call create() and
> kill() or are those just for data?

All the functions are static so no need for any inheritance or chaining.
I've also not used boost::noncopyable so that I can be happier with my
binary compatibility desire.

> If only boost::noncopyable used base class chaining to ensure
> this MI doesn't bloat the object (thus losing your binary
> compatibility goal).  Lacking that, perhaps the thing to do is to
> make the storage policy chainable:

Static functions.  Can worry about this if there is a sudden need I guess.

> 
> Consider yourself slapped!  This is rarely a good idea.
> 
> >     SizeType size( ) const
> >     {
> >         return number_of_things_;
>                  ^^^^^^^^^^^^^^^^^
> Wouldn't "size_" work?

size_ is good.  Done.
 
> >     SizeType number_of_bytes() const
> >     {
> >         return number_of_things_ * sizeof(Base);
> >     }
> 
> Perhaps "byte_count" would work?

Better.  Done.

 
> >     Base * overwrite( SizeType data_size, const Base * source )
> >     {
> >         assert(data_size <= MaxSize);
> >         size(data_size);
> >         return static_cast<Base *>(memcpy( data_, source,
> > number_of_bytes() ));
> 
> This duplicates logic already in some of the creation policies.
> Perhaps you should put "copy" in the suggested storage policy.
> (Another good reason for combining them!)

Even better, I think.  Got rid of the copy create thingo from the policy and
the constructor uses overwrite.

> >     typedef iterator_type< Base > iterator;
> >     typedef iterator_type< Base const > const_iterator;
> >
> >     iterator begin()   { return iterator(data_); }
> >     iterator end()   { return iterator(data_ + number_of_things_); }
> >
> >     const_iterator begin() const  { return const_iterator(data_); }
> >     const_iterator end() const { return const_iterator(data_ +
> > number_of_things_); }
> 
> Iterators are certainly compelling justification for this class.

The iterator_façade certainly makes it a whole lot easier.  I added a few
more stl'ish container typedefs and methods, but it is important to note
that this is _not_ a container, just smells like one.

> >     data( const data & source );
> >     data& operator=( const data & rhs );
> 
> Use boost::noncopyable.

Decided against using boost::noncopyable to leave no wriggle room on the
portability front for compilers without empty base class optimization.

> Separate issue: where are the typedefs, etc. for generic code?
> You should have data reveal its data type, its MaxSize, even its
> storage policy.  They're liable to be useful.

Done.

> > typedef data<>  data_standard;
> > typedef boost::shared_ptr< data_standard > data_standard_ptr;
> 
> These typedef's don't belong in this header.  Leave it to clients
> to establish their own idea(s) of what a "standard" version of
> data is.

Not 100% in agreement with this, but I don't feel that strongly about it.

Thanks Rob.  I've attached an improved version based on your comments and
guidance.  Thanks for your thoughtful comments.
 
The storage_allocate_and_kill bizzo is perhaps the most controversial
aspect.  I look forward to any comment w.r.t. this.

Just changed its name from data to raw_buffer as this perhaps explains what
it is about a little better.  Added it to the files section in case the
gmane monster eats it again: 

http://groups.yahoo.com/group/boost/files/raw_buffer.hpp 

Regards,

Matt Hurd.