[boost] Re: [serialization] load_construct proposal

From: David Abrahams (dave_at_[hidden])
Date: 2003-10-12 21:08:49

Robert Ramey <ramey_at_[hidden]> writes:

> Below I will try to summarize the argument
> on this thread.  I believe that mixing of unrelated
> issues has been very confusing.
>
> Here is the current default implementation:
> /////////////////////////
>
> template<class Archive ar, class T>
> void load(Archive & ar,  T * &t,  unsigned int file_version)
> {
>     t = NULL; 
>     try{  
>        t = new T();
>        ar >> *t;
>     }
>     catch(...){
>         delete t;

If you were to do it this way, you should do "t = NULL" here instead,
but...

>         throw;
>     }
> }
>

This should be 

template<class T, class Archive ar> // but see below
T* load(Archive & ar,  unsigned int file_version)
{
    std::auto_ptr<T> t(new T());
    ar >> *t;
    return t.release();
}

see http://www.boost.org/more/error_handling.html for rationale about
using auto_ptr instead of catch(...).  The T* return is a separate
issue: using a T*& out parameter makes no sense whatever to me, and
just seems to complicate matters, though of course I might have missed
something important... ohh, are you using it for overload resolution
so that users can provide an overload?  Using the same T* for both
purposes like that prevents functional programming and introduces
inefficiencies; I would prefer, e.g.:

template<class T, class Archive ar>
T* load(Archive & ar,  unsigned int file_version, T* = 0)
{
    std::auto_ptr<T> t(new T());
    ar >> *t;
    return t.release();
}

> Here is Dave Harris's proposal:
> /////////////////////
>
> template<class Archive ar, class T>
> void load_construct(Archive & ar,  T * t,  unsigned int file_version)
> {
>     new( pt ) T(); // Default-construct in place

Can't be right.  Where's the definition of pt?

>     ar >> *t;    // presumably
> }
>
>
> // the following would not be part of the public interface.

My understanding was that the following was as much part of the
public interface as load_construct above is.

> template<class Archive ar, class T>
> void load(Archive & ar,  T * &t,  unsigned int file_version)
> {
>     t = NULL; 
>     try{  
>        p = static_cast<T *>(new char[sizeof(T)]); // allignment issues?

Yes, should be 

  operator new(sizeof(T)) 

to avoid alignment problems.

>        load_construct(ar, static_cast<T *>vp, file_version)

Can't be right... where's vp?

>     }
>     catch(...){
>         delete t; // hmm - what about dtor? 

This is certainly wrong.  Should be

          operator delete(p);

>         throw;
>a     }
> }

  // untested!!

  // Detection for operator new and operator delete
  template <class T, T x> struct test;

  typedef char* yes;
  typedef int* no;
  typedef long* yes2;

  template <class T>
  yes has_op_new(T*, test<void* (*)(std::size_t), &T::operator new>* = 0);
  no has_op_new(...);

  template <class T>
  yes has_op_delete(T*, test<void (*)(void*), &T::operator delete>* = 0);
  template <class T>
  yes2 has_op_delete(T*, test<void (*)(std::size_t, void*), &T::operator delete>* = 0);
  no has_op_delete(...);

  // Handle the raw memory for inplace construction
  template <class HasNew = no, class HasDelete = no> 
  struct raw_memory_manager
  {
      template <class T>
      raw_memory_manager(T*)
         : memory(::operator new(sizeof(T)))
      {
      }

      void release() { void* m = memory; memory = 0; return m;  }

      ~raw_memory_manager()
      {
          ::operator delete(memory);
      }

      void* memory;
  };

  template <>
  struct raw_memory_manager<yes, yes>
  {
      template <class T>
      raw_memory_manager(T*)
         : memory(T::operator new(sizeof(T)))
      {
      }

      void release() { void* m = memory; memory = 0; return m;  }

      ~raw_memory_manager()
      {
          T::operator delete(memory);
      }

      void* memory;
  };

  template <>
  struct raw_memory_manager<yes, yes2>
  {
      template <class T>
      raw_memory_manager(T*)
         : memory(T::operator new(sizeof(T)))
         , size(sizeof(T))
      {
      }

      void release() { void* m = memory; memory = 0; return m;  }

      ~raw_memory_manager()
      {
          T::operator delete(size, memory);
      }

      void* memory;
      std::size_t size;
  };

  // no specializations for probably-broken yes/no mixtures.

  // guts of load implementation
  template<class Archive ar, class T, class HasNew, class HasDelete>
  T* load_impl(Archive & ar,  T *,  unsigned int file_version, HasNew, HasDelete)
  {
       raw_memory_manager<HasNew,HasDelete> m;
       load_construct(ar, static_cast<T*>(m.memory), file_version);
       return static_cast<T*>(m.release());
  };

template<class Archive ar, class T>
T* load(Archive & ar,  T *,  unsigned int file_version)
{
   return load_impl(
        ar, (T*)0, file_version
      , has_op_new((T*)0), has_op_delete((T*)0)
   );
}

> The current system
>
> 1) simpler - transparently correct

Definitely.

> 2) permits any required overload
> 	a) memory allocation

I don't see any provision for construction of an arbitrary type in
raw memory.

> 	b) construction

Yes.

> 	c) exception handling.

What does EH have to do with overloading?

> 3) maintains analogy with save/load nomenclature for seialization of objects.
>
> Your proposal
>
> 1) is not as transparently correct.

Definitely.

> 2) raises issues as to inplace construction alignment.

Dispatched.

> 3) without making the public api larger, does not permit
> 	a) override of memory allocation

I think it does.

> 	b) overide of exception handlling

WHat does that mean?

> 4) replace overriable functions save/load with  save_construct/load_construct.
> These don't convey meanings of what they do. ( what could save_construct
> possibly mean?)

save_construct isn't needed.  I think the overridable functions
should be save, load (what we've called load_construct 'till now), and
dynamic_load or load_allocate *IF* we even want that function to be
overridable... of which I'm not convinced.

> Note that this issue has absolutly nothing to do with the ability to
> override and/or access private default constructors.  Both systems
> use the default constructor as the er... default constructor. Both
> systems require the user to specifiy an override if there is no
> default constructor.  Neither system currently permits access to a
> private default constructor.  I will address the issue of access to
> a private constructors - default or otherwise in a separate post.
>
> So, in light of the above, I'm not convinced that any change would be beneficial.

That may still be true, though I'm not sure.  It seems like the
ability to override inplace construction is important for
deserializing arrays.

-- 
Dave Abrahams
Boost Consulting
www.boost-consulting.com