|
|
Boost : |
From: Rob Stewart (stewart_at_[hidden])
Date: 2004-03-26 10:27:36
From: "Matthew Hurd" <matt_at_[hidden]>
> > On Behalf Of Rob Stewart
>
> Fancy meeting you here. We work at the same company, SIG, just on
> different
> sides of the world. I work on the top side in Sydney and you work down
> north ;-)
I suspected it was you, but didn't want to assume it given the
non-SIG address.
> > 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.
> 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.
> data<> usual;
> data<256, create_refer, kill_delete > take_responsibility_for;
> data<256, create_new, kill_delete > page6502;
> data<4096, create_new, kill_delete, __int8, __int64 > vm_page;
> data<MTU_SIZE, create_refer, kill_free, char*, __int64 > dodgy;
> data<1e10, create_refer, kill_free, char*, __int64 >
> big_sucker_zero_copy;
>
> data_standard_ptr dont_copy___share_the_love; // non-copyable
>
> I've found the iterator on the buffer via the neat
> boost::iterator_facade is
> handy syntax sugar too. (If I did it correctly, I was a facade
> virgin.)
>
> /*---------------------------------------------------------------------
> created: 2004-3-24 18:30
> filename: data.hpp
> author: Matt Hurd
>
> purpose: I'm sick of getting confused about who owns what buffer
> as I'm not too clever about remembering stuff.
>
> This 7 way configurable data buffer that 'allows' binary
> compatibility with a WSABUF on win32 solves a few simple
> but annoying issues for me.
>
> Also I've found it has acted as nice simple intro for
> a couple of colleagues w.r.t. to policy based design.
>
> 7? 3 create policies and 3 delete policies == 9 options
> 2 don't make sense, new with free and malloc with delete.
> 7 that could make sense though I'm not sure the malloc
> with free should ever be used when you have a perfectly
> good new with delete ;-)
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.
> PS: A vector<Base> won't do because of the ownership
> transfer semantic issue when integrating with third
> party libraries.
> -----------------------------------------------------------------------*/
>
>
> #ifndef DATA_HPP_2004324
> #define DATA_HPP_2004324
>
> #include <boost/shared_ptr.hpp>
> #include <boost/static_assert.hpp>
> #include <boost/iterator/iterator_facade.hpp>
>
> namespace net {
> //-----------------------------------------------------------------------
> ----
> // Create policies
>
> // new space, allocate with new
> template < int MaxSize, class T, typename SizeType >
Why restrict MaxSize to be of type int? You can eliminate
SizeType; see below.
> struct create_new
> {
> static T * create()
> {
> return new T[MaxSize];
> }
>
> static T * create( const T * source, SizeType size)
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.
> {
> return static_cast<T*>(memcpy( create(), source, size *
> sizeof(T) ));
Um, what if size > MaxSize?
> }
> protected:
> ~create_new() {}
> };
>
> // new space, allocate with malloc
> template < int MaxSize, class T, typename SizeType >
> struct create_malloc
> {
> static T * create()
> {
> return static_cast<T*>(malloc(MaxSize * sizeof(T) ));
> }
>
> static T * create( const T * source, SizeType size)
> {
> return static_cast<T*>(memcpy( create(), source, size *
> sizeof(T) ));
Same problem here.
> }
> 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.)
> {
> 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?
> }
>
> static T * create( T * source, SizeType size)
> {
> return source;
> }
> protected:
> ~create_refer() {}
> };
>
>
> //-----------------------------------------------------------------------
> ----
> // Kill policies
>
> // data owns the block, allocated with new T[]
It's comments like that that make me want to combine the
policies.
> template< class T >
> struct kill_delete
> {
> static void kill( T * p )
> {
> delete [] p;
> }
> protected:
> ~kill_delete() {}
> };
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() {}
};
> // data owns the block, allocated with malloc
> template< class T >
> struct kill_free
> {
> static void kill( T * p )
> {
> free(p);
> }
> protected:
> ~kill_free() {}
> };
>
>
> // data is not to own the data block, don't delete it
> template< class T >
> struct kill_nothing
> {
> static void kill( T * p )
> {
>
> }
>
> protected:
> ~kill_nothing() {}
> };
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?
template < typename T, typename MaxSize >
struct storage_reference_only
{
template < typename SizeType >
static T * create(T * source, SizeType size)
{
BOOST_ASSERT(size <= MaxSize);
return source;
}
static void kill(T * p)
{
}
protected:
~storage_reference_only() {}
};
> //-----------------------------------------------------------------------
> // The data
>
> template<
> size_t MaxSize = 4096
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.
> , 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.
> , 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?
> Base >
My version:
template
< typename MaxSize
, template < typename, typename > class StoragePolicy =
storage_array_new
, typename T = char
>
class data : StoragePolicy< MaxSize, T >, boost::noncopyable
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:
namespace detail
{
struct end_of_base_class_chain { };
}
template
< typename T
, typename MaxSize
, class Base = detail::end_of_base_class_chain
>
struct storage_example : Base
{ ... };
> // implicit conversion is a bad idea, but I'm addicted to its
> handiness, someone slap me ;-)
> operator Base * ()
> {
> return data_;
> }
Consider yourself slapped! This is rarely a good idea.
> SizeType size( ) const
> {
> return number_of_things_;
^^^^^^^^^^^^^^^^^
Wouldn't "size_" work?
> SizeType number_of_bytes() const
> {
> return number_of_things_ * sizeof(Base);
> }
Perhaps "byte_count" would work?
> 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!)
> 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.
> private:
> // non copyable - if you're thinking of copying:
> // a) think again,
> // b) use a shared_ptr<data...> to reference
> count
> // c) if you really have to have the same data
> twice
> // use the constructor to clone
>
> data( const data & source );
> data& operator=( const data & rhs );
Use boost::noncopyable.
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.
> 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.
-- Rob Stewart stewart_at_[hidden] Software Engineer http://www.sig.com Susquehanna International Group, LLP using std::disclaimer;
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk