Re: [boost] Serialization Library Review

From: Vladimir Prus (ghost_at_[hidden])
Date: 2002-11-11 12:08:26

David Abrahams wrote:

> Here's a reminder of what to include in your review comments:
>    Here are some questions you might want to answer in your review:
> 
>       What is your evaluation of the design?

First comes my opinion on library scope. The biggest features that it
adds (compared with older 'describe' scheme) are saving of pointers and
versioning.

There are other interesting things, for example, creating
human-readable archives with field/variable names. E.g. I might want to
create a dump file which can be directly read by Python (and should use
Python syntax for lists/maps). Those things are not supported, but I won't
discuss them for now --- it's a topic for future.

Regarding pointer saving, I was posting quite a lot, but was unable to
convince Robert there's a problem. Briefly, if some module saves pointers to
polymorphic class B, then it should *include headers* that declare *all classes
derived from B*. I don't think this requirement is reasonable.

More details can be found in:
http://aspn.activestate.com/ASPN/Mail/Message/1265629
http://aspn.activestate.com/ASPN/Mail/Message/1358054

I don't like the 'serialization' class, as well as arguments for putting
it in global namespace. The argument is that if it were in 'boost' namespace,
then to declare a specialization, one would have to open 'boost' namespace.
However, I don't declared my classes in global namespace, so to add a specialization,
I'd have to do that outside of my namespace, which is not better. In fact, I'd
*strongly* prefer free-standing functions.

It appears that under "Serialization of templates", the reference suggests
to declare something in namespace boost::serialization_detail. I think that
(1) that should be boost::serialization::detail and (2) user should not
declare anything in detail namespace.
Also, the natuaral syntax at the top of that section seems correct to me.
If it is really not possible, then it should be told why.

I would like to avoid declaring "version" function when I don't want
to use any versioning, althought I don't see how to do this in the current
library.

> 
>       What is your evaluation of the implementation?

I'm not overly happy with the implementation --- it took me
quite a lot to understand what's going on there. Lack of
comments for internal classes cause some of the problems.

There's missing typename at
     ../../boost/serialization/stl.hpp:124

There's line
     assert(is_polymorphic<T>::value).
I guess this should be compile-time assertion? And, BTW, is
typeid of *t is not equal to static type of *t, then *t is
already polymorphic, unless I'm missing something.

> 
>       What is your evaluation of the documentation?

Single-level index for reference docs makes nagivation hard.

Documentation does not say that "version" must be *static* member
function.

Rationale section talks about "saved objects must be const". I don't
understand what this means, and the example from reference for this
part compiles fine for me, while docs say it should not.

The 'register_type' method is documented only inside docs for particular
kind of exception. It should be moved elsewhere. Storing of base pointers
is in generally underdocumented.

The following looks overly generic:
     template<
         class T,
         class Allocator,
         template Container<class T, class Allocator>
     >
     boost::basic_oarchive & ::operator<<(
         boost::basic_oarchive & ar,
         const Container<T, Allocator> & t
     );
it would catch all classes which happen to be instances of templates with
two parameters. The phrase "STL containers can be serialized using above syntax"
is very confusing. I belive it should be stated that all STL containers can
be serialized, without the interface above.

> 
>       What is your evaluation of the potential usefulness of the
>       library?

I believe that the issue with storing polymorphic pointers makes the library
not very usefull. In effect, to store polymorphic pointers, I have to
implement registraction system and make loading of base classes create
derived classes using registered id. This almost amounts to implementing my
own serialization library.

>       Did you try to use the library?  With what compiler?  Did you
>       have any problems?

Yes, with g++ 2.95.4 and 3.2 (although lack of Jamfile required manual compilation).
It did not compile, saying that:

../../boost/void_cast.hpp:28: redefinition of `struct boost::is_polymorphic<T>'
/home/ghost/Work/boost/boost/type_traits/is_polymorphic.hpp:75: previous
    definition of `struct boost::is_polymorphic<T>'
In file included from ../../boost/serialization/serialization_imp.hpp:16,
                  from ../../boost/serialization/archive.hpp:4,
                  from archive.cpp:17:
../../boost/void_cast.hpp:28: redefinition of `struct boost::is_polymorphic<T>'

After commenting out part of void_cast.hpp, it did compile. However, the attached
program produced no output with 3.2. The line 36 simply did nothing ("step" in gdb
on it did not lead anywhere).

> 
>       How much effort did you put into your evaluation? A glance? A
>       quick reading? In-depth study?

In-depth. I've been closely watching the library development from
the very first post on the ml.

>       Are you knowledgeable about the problem domain? 

Can't say definitely. However, I've looked in some other
implementation, so must know something.

>    And finally, every review should answer this question:
> 
>       Do you think the library should be accepted as a Boost library?
>       Be sure to say this explicitly so that your other comments don't
>       obscure your overall opinion.

I think the library should not be accepted. I don't like to vote this
way, but I'm not convinced that serialization library with such requirement
for storing polymorphic pointers is really ready to go.

- Volodya