RE: [boost] Serialization Library Review

From: Jeff Garland (jeff_at_[hidden])
Date: 2002-11-16 19:50:15

I have spent several hours looking at the library.  In that time I read the docs, skimmed the implementation, tried compiling the
tests, and wrote a simple sample program.  I have written more than one serialization library, so I am pretty familiar with the
domain.  Unfortunately, I didn't have much time to really look at the implementation so my comments there are somewhat limited.  I
used gcc3.2 on Mandrake Linux 9 for testing.

Before I launch into the review I want to say thanks to Robert all his good effort on this -- it is a massive and extremely
difficult undertaking!

And, also a request that we try to schedule more time for library reviews -- 9 days just isn't enough given that this has to be
fit in with everything else.

I'm going to abstain on voting for or against inclusion because I'm impossibly conflicted.  While I certainly have several
reservations, I don't think (but I'm not sure) that the library is so flawed as to be unfixable.  And overall this library
represents a solid effort that can provide a nice foundation for future work. If we reject the library Robert may just quit which
would be tragic b/c this is a critically important library.  On the other hand, my inability to compile all the tests makes me
question if the library is ready for prime time.  Note that I haven't followed all the other review issues closely so I won't
comment on those.

>From my perspective the following changes must are must-haves:
1) The library must have a 'real' test suite that checks.  We will never get this
   ported without some automated tests. Further, it is simply too much to ask users
   to use this library without some tests that show that it works on their
   platform/compiler.
2) I'm concerned that gcc 3.2 won't compile the 'tests' out of the box (see below)
3) While the demo is the 'big' complete example, there needs to be a 10 synopsis
   example using standard types.  Programmers understand code faster than prose.
4) Jamfiles that build a library using the boost standard directory form.  It took me a
   good 30 minutes to realize that my linking error was a result of a failure link
   the archive.o, serialization.o, and void_cast.o.  The combination of so many
   header libraries in boost and the lack of directory definition in under /lib
   put me in the wrong mindset.
5) Having to hack changes (make operator<<'s public) into archive.hpp for a simple
   program to archive shouldn't be required.  I wasn't sure if this was a design
   flaw or the intent.

That's it for the summary.  Here are some detailed comments:

--- Design ---

The fact that the archive is not required to be implemented using an streams is a plus.  I can easily imagine wanting to use
serialization with a database backend.

Requiring a default constructor is a mistake.  For one thing there are often reasons not to provide a default constructor.  For
another, when serializing we might want different behavior.  Provide a factory method instead.

Question, do classes without default constructors must be serializable -- if so, I would add this to my list of must have changes.

--- Documentation ---
* I ran link_checker on the docs -- no problems.

* You should reference the serializer pattern http://www.riehle.org/computer-science-research/1996/plop-1996-serializer.html even
if you didn't know about it :-)

* Copyright is incorrect.  If you Reserve All Rights this means that others cannot
  distribute or modify the document which is not consistent with the boost philosophy.

* Running the docs thru the W3C validation service (some results below) which reveals
  a number of bugs in the html.  I got bored doing this, so I'll let you do the rest
  yourself.  http://validator.w3.org/

* "Code to serialize this container (and all the others) is found int serialization.hpp"
  ---> 'int' should be 'in'

--- Implementation ----
* This library needs a test-suite so that it can be tested.
* Jamfiles are needed for the examples.
* Libs should follow the standard directory structure:
  Libs/Serialize/example
  Libs/Serialize/build
  Libs/Serialize/src
  Libs/Serialize/doc

  The directories msvs7, mingw, and comeau should be below the build
  directory.

* Why not derive a different exception from archive_exception for each of the
  enum types instead of using the enum?  This would be better because if a user
  creates a new archive type they might need to add an exception type which is
  not currently possible.

*********************** Compile Failures       ***********************************

############################################
demo_shared_ptr.cpp

g++ -I /home/jeff/devTools/boost_rc_1_29 -I ../../.. -c ../demo_shared_ptr.cpp
In file included from ../demo_shared_ptr.cpp:14:
../shared_count.hpp:198: warning: friend declaration `void boost::serialization_detail::save_ptr_template(boost::basic_oarchive&,
const boost::detail::counted_base_impl<P, D>*, long int)' declares a non-template function
../shared_count.hpp:198: warning: (if this is not what you intended, make sure the function template has already been declared and
add <> after the function name here) -Wno-non-template-friend disables this warning
../demo_shared_ptr.cpp:274:2: warning: no newline at end of file
/home/jeff/devTools/boost_rc_1_29/boost/shared_ptr.hpp: In function `void
boost::serialization_detail::load_template(boost::basic_iarchive&, boost::shared_ptr<T>&, unsigned char, long int) [with T = A]':
../../../boost/serialization/serialization_imp.hpp:315:   instantiated from `void
boost::serialization_detail::type_info_class_load<T>::load_object_data(boost::basic_iarchive&, void*, unsigned char) const [with T
= boost::shared_ptr<A>]'
/home/jeff/devTools/boost_rc_1_29/boost/mpl/if.hpp:59:   instantiated from here
/home/jeff/devTools/boost_rc_1_29/boost/shared_ptr.hpp:262: `A*boost::shared_ptr<A>::px' is private
../demo_shared_ptr.cpp:197: within this context
/home/jeff/devTools/boost_rc_1_29/boost/shared_ptr.hpp:263: `boost::detail::shared_count boost::shared_ptr<A>::pn' is private
../demo_shared_ptr.cpp:198: within this context
/home/jeff/devTools/boost_rc_1_29/boost/shared_ptr.hpp: In function `void
boost::serialization_detail::save_template(boost::basic_oarchive&, const boost::shared_ptr<T>&, long int) [with T = A]':
../../../boost/serialization/serialization_imp.hpp:285:   instantiated from `void
boost::serialization_detail::type_info_class_save<T>::save_object_data(boost::basic_oarchive&, const void*) const [with T =
boost::shared_ptr<A>]'
/home/jeff/devTools/boost_rc_1_29/boost/mpl/if.hpp:59:   instantiated from here
/home/jeff/devTools/boost_rc_1_29/boost/shared_ptr.hpp:262: `A*boost::shared_ptr<A>::px' is private
../demo_shared_ptr.cpp:182: within this context
/home/jeff/devTools/boost_rc_1_29/boost/shared_ptr.hpp:263: `boost::detail::shared_count boost::shared_ptr<A>::pn' is private
../demo_shared_ptr.cpp:183: within this context
make: *** [demo_shared_ptr.o] Error 1
############################################
demo_auto_ptr.cpp

g++ -I /home/jeff/devTools/boost_rc_1_29 -I ../../.. -c ../demo_auto_ptr.cpp
../demo_auto_ptr.cpp:120:2: warning: no newline at end of file
../../../boost/serialization/serialization_imp.hpp: In member function `version_type
boost::serialization_detail::type_info_class_save<T>::version_object(void*) const [with T = std::auto_ptr<A>]':
/home/jeff/devTools/boost_rc_1_29/boost/mpl/if.hpp:59:   instantiated from here
../../../boost/serialization/serialization_imp.hpp:279: choosing `version_type boost::serialization_detail::version_template(const
std::auto_ptr<T>&, long int) [with T = A]' over `version_type boost::serialization_detail::version_template(T&, int) [with T =
std::auto_ptr<A>]'
../../../boost/serialization/serialization_imp.hpp:279:   because worst conversion for the former is better than worst conversion
for the latter
make: *** [demo_auto_ptr.o] Error 1

############################################
test.cpp

g++ -I /home/jeff/devTools/boost_rc_1_29 -I ../../.. -c ../test.cpp
../test.cpp: In constructor `F<Container, Compare>::F()':
../test.cpp:698: parse error before `=' token
../test.cpp: In member function `void F<Container, Compare>::save(boost::basic_oarchive&) const':
../test.cpp:744: parse error before `>' token
../test.cpp: In member function `void F<Container, Compare>::load(boost::basic_iarchive&, unsigned char)':
../test.cpp:754: parse error before `>' token
../test.cpp: In constructor `F<Container, Compare>::F() [with Container = std::set<A*, ptr_less<A*>, std::allocator<A*> >, Compare
= A]':
../test.cpp:1266:   instantiated from `int outer_test(TS) [with TS = char_test]'
../test.cpp:1348:   instantiated from here
../test.cpp:701: `t' undeclared (first use this function)
../test.cpp:701: (Each undeclared identifier is reported only once for each function it appears in.)
make: *** [test.o] Error 1

############################################

*********************** Detailed Documentation Issues ***********************************
++index.html++

<dt><a href="reference.html">Reference</dt>
                                              ^
Error: end tag for "A" omitted; possible causes include a missing end tag, improper nesting of elements, or use of an element
where it is not allowed

Line 26, column 6:
    <dt><a href="reference.html">Reference</dt>
        ^

Error: start tag was here

++overview.html++

Line 112, column 3:
  </p>
     ^
Error: end tag for element "P" which is not open; try removing the end tag or check for improper nesting of elements

++rational.html++
Line 74, column 35:
  ... jects must be <code>const</code></a></h2>
                                         ^

Error: end tag for element "A" which is not open; try removing the end tag or check for improper nesting of elements

Line 105, column 3:
  </p>
     ^

Error: end tag for element "P" which is not open; try removing the end tag or check for improper nesting of elements

Line 120, column 3:
  </p>
     ^

Error: end tag for element "P" which is not open; try removing the end tag or check for improper nesting of elements

################################################################################

My simple test program which required hacking of archive.hpp operators from
private to public to work....

#include <boost/serialization.hpp>
#include <string>
#include <fstream>
#include <list>

void save()
{
  std::string foo = "bar";
  const int anInt = 100;
  std::list<std::string> sl;
  sl.push_back("string_one");
  sl.push_back("string_two");
  sl.push_back("string_three");
  std::ofstream ofs("test_string.out");
  boost::oarchive oa(ofs);
  oa << anInt << foo << sl;
}

void restore()
{
  std::string foo;
  int anInt;
  std::list<std::string> sl;
  std::ifstream ifs("test_string.out");
  boost::iarchive ia(ifs);
  ia >> anInt;
  ia >> foo;
  ia >> sl;
  std::cout << sl.size() << std::endl;
  std::cout << foo << std::endl;
  std::cout << anInt << std::endl;
}

int
main()
{
  save();
  restore();
}

Whew, that's it!

Jeff