Re: [boost] serialization: review

From: Robert Ramey (ramey_at_[hidden])
Date: 2004-04-19 14:59:33

Thorsten Ottosen wrote:

> Why isn't there a base class for all archieves? I think that would be
useful

> with class hierachies...for example, I need to implement two function and
> cannot use & anymore. Consider this

> struct mammal : boost::noncopyable
> {
> private:
>     friend class boost::serialization::access;
>     virtual void do_serialize( boost::archive::text_oarchive& ar, unsigned
> version ) = 0;
>     virtual void do_serialize( boost::archive::text_iarchive& ar, unsigned
> version ) = 0;
>     template< class Archive >
>     void serialize( Archive & ar, unsigned version )
>     {
>         do_serialize( ar, version );
>     }
> public:
>     virtual const std::string& name() const = 0;
>     virtual unsigned           age() const  = 0;
>     virtual ~mammal() {}
> };

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.
Just declare it as abstract using BOOST_IS_ABSTRACT(mammal).  Only the
serialization of the derived classes need be specified. 

> I can't figure out exactly how the library save/loads pointers when these
> cannot be allocated on the stack.

Please expand some more on this.

>   a.. What is your evaluation of the documentation?

> I miss more specific type requirements. For example, is it necessary to
> have a default constructor? Is it necessary to be stack-allocable? Must
> the destructor be public? 

Good point.

> I did try to extend the library to use my new smart containers; I was
> almost successful.

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

> 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.

> I tried to extend the library to support my smart containers, but I
> couldn't figure out what I did wrong. I think I might have saved them
> correctly, but I cannot figure out to load them. However, I do think that
> is a problem that can be solved together with Robert. (Robert, you can
> find the ptr_vector in the sandbox. If you could cast a glance on the
> code and tell me how the body of load_collection should look like, it 
> would be great.)

I'll look at it.

Thorsten

> Some general nitpicks/comments (with these comments my test works on vc71,
> como4.3.3, g++3.3.1:

> 1. have you planned to use concept checks

I'm intrigued with concept checks for something like this but I havn't had
the time to spend on it.

> 2. documentation: can the width of the html be made adaptive to the window
> width? The right window must be scrolled to see it's contents which is
> annoying.

This drives me crazy also.  Unfortunately, I haven't been able to figure out
what causes this.  I believe it's a Microsoft IE issue - but who knows.  I
would be grateful to anyone can explain to me how to fix this.

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.

> 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.  

> 4. AFAICR, the use of placement new require proper alignment, so the
> object must eg. be heap-allocated. Is that guaranteed?

I believe this has been addressed. Objects allocated on the stack use
aligned storage to handle this.  

> 5. I think the exception code should be returned by a function:
> exception_code code() const; this is how eg. the regex_error exception in
> tr1 will do it.

> 6. remove trailing comma in archive_exception.hpp: "unregistered_cast  //
> base - derived relationship not registered with"

OK - though I don't think its an error.

> 7. archive/detail/basic_oarchive.hpp includes itself

Whoops.

> 8. is_abstract.hpp: there is something wierd about the is_abstact struct. 
> uncommented the first part to be able to compile.

I don't see this.

> 9. serialization/serialization.hpp: why all the static casts?

> template<class Archive, class T>
> inline void serialize(
>     Archive & ar, T & t, const BOOST_PFTO unsigned int file_version
>){
>    access::serialize(ar, t, static_cast<const unsigned
int>(file_version));
> }

> AFAICT, they should be removed. Comeau has the follwing warning:  type
> qualifier is meaningless on cast type

They wouldn't be there except they provide a work around for compilers which
do not support Partial Template Function Specialization.  On conforming
compilers, the BOOST_PFTO resolves to nothing so the cast wouldn't be
necessary.  So commeau is correct.  I don't know how much effort I want to
invest to suppress benign warnings for specific compilers.

> 10. nvp.hpp: line 50: 
> ar & t.value();      
> There is on t-parameter.

OK - interesting the default implementation was never instantiated.

> 11. /archive/detail/oserializer.hpp", line 244:   NULL reference is not
> allowed
>   return * static_cast<const basic_pointer_oserializer *>(NULL);
                     ^
> This not yet legal C++ (but the committee is changing that) use a static
> member and return that instead.

> Also, why all the static_casts?

In this case the reason is that when handling issues regarding preamble,
object-id, class id etc. the pointer to the object is handled as void * .
This permits the archive library to be pre-compiled and not have to be
recompiled for every instance.  When such a void pointer is passed back to a
specific archive-type combination it has to be case back to the orginal type
to permit the calling of type serialization functions. This is really a
result reconciliation of conflicting goals. 

a) serialization of pointers abstract and otherwise
b) serialization of pointers in a non-intrusive manner
c) limit pointless instantiation.
d) not require that user classes import headers related solely to
implementation. 

For pointers this has generated a design which takes the typed pointer
converts it to void pointer, handles preamble and book keeping, converts it
back to typed pointer and (de)serializes the data.  The requirement of
non-intrusiveness means we can't just pass around a base class pointer -
hence the need for casting.

> 12. two-phase lookup problems:
> /boost/archive/basic_text_oarchive.hpp", line 65: error #20:
>           identifier "This" is undefined
>           archive::save(* This(), t);
                          ^
> Solution: prefix base class functions with this->. 

I'm not sure I understand this but I don't see it doing any harm.  If this
is a problem, the following would make more sense to me.

    archive::save(* common_oarchive<Archive>::This(), t);

> remark: why is this->This()->XX() necessary? this->xx() should do it.

Hmm - I don't see this.  save(Archive & ar, T t) is implemented in a more
derived class. Common_oarchive<Archive> rededirects it downward to that
class. I would expect this->xx() to give and undefined error.  I could use

    archive::save(* static_cast<Archive *>(this)t);

though I'm not sure you would find that anymore appealing.

> 13: archive/text_oarchive.hpp, similar lookup problems. this->newtoken(),
> this->delimiter = this->none;

as before - I don't see a problem but changing it doesn't hurt anything.

> 14: /archive/detail/basic_iarchive.hpp", line 46: error #20:
>           identifier "basic_iarchive_impl" is undefined
>       boost::scoped_ptr<basic_iarchive_impl> pimpl;
                        ^
>solution: remember to forward declare basic_iarchieve_impl. Do the same for
>basic_oarchieve_impl.

OK - I had thought that using friend class made that unnecessary.

> 15: /archive/detail/archive_pointer_iserializer.hpp", line 42: error #284:
> (+detail/iserializer.hpp", line 240:)
>           NULL reference is not allowed
>           return *static_cast<const basic_iserializer *>(NULL);
                 ^
> Solution: use a static member again.

> 16: two-phase lookup problems in archive/basic_text_iarchive.hpp", 
> Solution: prefix by this->.

> 17. when you save_collection_impl.hpp (eg in vector.hpp) there is no need
> to specify template arguments again since they will be deduced.

I presume you mean that:

template<class Archive, class U, class Allocator>
inline void save(
    Archive & ar,
    const std::vector<U, Allocator> &t,
    const unsigned int file_version
){
    stl::save_collection<Archive, std::vector<U, Allocator> >(
        ar, t
    );
}

Can be replaced with:

template<class Archive, class U, class Allocator>
inline void save(
    Archive & ar,
    const std::vector<U, Allocator> &t,
    const unsigned int file_version
){
    stl::save_collection<Archive, std::vector>(
        ar, t
    );
}

But what about:

template<class Archive, class U, class Allocator>
inline void load(
    Archive & ar,
    std::vector<U, Allocator> &t,
    const unsigned int file_version
){
    stl::load_collection<
        Archive,
        std::vector<U, Allocator>,
        stl::archive_input_seq<Archive, std::vector<U, Allocator> >,
        stl::reserve_imp<std::vector<U, Allocator> >
    >(ar, t);
}

This can't be replaced with:

template<class Archive, class U, class Allocator>
inline void load(
    Archive & ar,
    std::vector<U, Allocator> &t,
    const unsigned int file_version
){
    stl::load_collection<
        Archive,
        std::vector
        stl::archive_input_seq<Archive, std::vector>,
        stl::reserve_imp<std::vector>
    >(ar, t);
}

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

> 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 . 

> 19. missing #include <boost/serialization/nvp.hpp> in serialization.hpp

Whoops - thanks.

Robert Ramey