Re: [boost] Re: serialization: review

From: Robert Ramey (ramey_at_[hidden])
Date: 2004-04-20 10:59:11

Thorsten Ottosen wrote:

> Robert Ramey wrote:(> |)

> | I don't understand the need for this. Given that mammal is an abstract
> | base class with no state variables of its own, it needs no serialization
> | at all.

> Ok, but could I have implemented it by having just one function in the 
> abstract base class which seriaized by calling virtual functions, eg,
> name() and age()?

I believe you could.  In fact I believe that in some cases not yet
encountered ( runtime plug-ins ) it will be necessary.  This is still an
unresolved area

> | Just declare it as abstract using BOOST_IS_ABSTRACT(mammal).  Only the
> | serialization of the derived classes need be specified.

> Can BOOST_IS_ABSTRACT be avoided? Maybe some type-traits can figure this
> out?

Is_abstract is in the new type traits. It fails on lots of compilers.  Also
it was in a state of flux at the time of the release of 1.31.  I expect that
a) is_abstract get fixed up best it can
b) and have an implementation that is benign for compilers that don't
support it
c) and that we'll continue to specialize it for serialization code that
needs to be compiled on less-conforming compilers. (this is all
BOOST_IS_ABSTRACT does.)

In
> struct whale : public mammal
> {
>     friend class boost::serialization::access;
>     whale();
>     whale( const std::string& name, unsigned age );
>     virtual const std::string& name() const;
>     virtual unsigned           age() const;
> private:
>     template<class Archive>
>     void serialize(Archive &ar, const unsigned int version){
>         ar & boost::serialization::base_object<mammal>(*this);
>         ar & name_ & age_ ;
>     }
>     std::string   name_;
>     unsigned      age_;
> };

> can the line "ar & boost::serialization::base_object<mammal>( *this )"
> be put in a base-class like this

> mammal::serialize( Archive& ar, unsigned version ) {
>         ar & boost::serialization::base_object<mammal>(*this);
>         do_serialize( ar, version ); 
>         // virtual function that does the rest
> }

> to remove some redundancy?

The intention of base_object is really to cast to the base class (and
register the base/derived relationship).  So it its out of place here. The
real issue is do_serialize. Is that supposed to be a virtual function? With
a templated argument? Maybe you want to consider this question a little bit
more.

> | I would love to hear more about this.  I would hope that it would easy 
> | to be totally successful.

> it is now given you showed me how :-) In effect, once the container 
> supports serialization, a class can be supported in four lines. If you 
> have not got an example of class hierarchies, then maybe it should be 
> added.

The tutorial contains an example essentially identical to your mammals,
whales and apes example.

Under case studies there's a discussion of the implementation of
serialization for shared_ptr which is almost exactly analogous to your
vector_ptr case.

Also, there some discussion in the documentation regarding serializaition of
templates.

> | > I would like to see the focus shift from vector<T*> towards vector< 
> | > shared_ptr<T> > and ptr_vector<T>. using T* array[sz] or vector<T*> 
> | > is inherently unsafe if the pointers are heap-allocated.
> |
> | I believe that enforcing this is outside my role as a library creator.

> ok, but I think you at least should point out very clearly the problems in
> it. Some non-experienced programmer might do what he sees in 
> documentation; therefore examples should encourage good style.

Well, here is one programmer that needs to be better sold on this.
Honestly, I don't think the serialization tutorial is the place to do it.

> | 3. const members uses a const-cast to re-init them. AFAICT, this is 
> | undefined behavior (cmp 7.1.5.1). I guess the need for serialization 
> | is just another reason for not having const members.

Hmmm - what does re-init mean in this context?

> | > this has come up from time to time.  I've tried to promote the view 
> | > that const members shouldn't be serialized but I couldn't convince
> | > everyone.
> | > I consider it outside my role as a library developer to try to 
> | > enforce this.

> my point was that the examples given give undefined behavior so that
> solution cannot be used.

Which example gives undefined behavior?  

> no, but maybe with this

> |         stl::archive_input_seq<Archive, std::vector>,
> |         stl::reserve_imp<std::vector>

> by arranging the order of the template parameters.

> | Isn't a little bit of redundancy OK giving the alternative of having 
> | to keep the details of deducibility in one's brain?

> but you already know the types that will be deduced from the outer
> function.

OK, could we agree to disagree on this and forget about it?

> | > 18. implementation of save_collection(). you cannot use unsigned int
> | > count portably.
> |
> | I don't see this.
> |
> | > Instead you should use the new collection traits (reviewed after 
> | > this review) as follows:
> |
> | > typename boost::size_type_of< Collection >::type count = size( s );
> |
> | > then the code will also work for iterator views and arrays. Similar 
> | > comments apply to load_collection_impl.hpp
> |
> | I wanted to make the library buildable under 1.31 .

> ok, then do

> typename Collection::size_type count = s.size();

> but don't assume Collection::size_type will be unsigned int.

I'm not assuming that. I'm assuming that it will be convertible to an
unsigned int and that an unsigned int will be large enough to hold any
number of elements in an collection. Of course THAT might be rash
assumption.  BTW where is the namespace Collection defined in 1.31 ?

Robert Ramey