Re: [boost] Serialization Formal Review #2

From: Matthias Troyer (troyer_at_[hidden])
Date: 2004-04-24 15:34:44

On Apr 12, 2004, at 10:09 PM, Jeff Garland wrote:
> 2) I am particularly interested in hearing from parties that have 
> tried to
> extend the library by adding serialization for a new type or by 
> building a new
> type of archive.  The ability to easily extend the library for new 
> types and
> novel types of archives is critical for this library.

I have tried extending the library since I want to swap our old 
serialization library used at http://alps.comp-phys.org with a Boost 
serialization library as soon as possible. It was important to keep the 
same archive format, since we have gigabytes of data collected over ten 
years in millions of CPU-hours and need to be able to read those with 
both new and old codes. With the help of Robert I managed to convince 
myself that this is indeed possible. The only issue I have is that the 
documentation needs to be improved.

> 3) If you reviewed the library during the first review, I would like 
> your
> evaluation of whether the new version of the library addresses your 
> original
> concerns.  Also, if this is a re-review I would appreciate if you 
> could send
> me a weblink from the mail archive to your original review posting(s).

In the first round I voted against inclusion in Boost because i) the 
design was not flexible enough to allow reading my old legacy formats 
and ii) the design inhibted some optimizations I need for large data 
sets. Robert did an excellent job in addressing this and other issues. 
The new version is greatly improved and I am satisfied with the design.

> As usual, please state in review comments how you reviewed the library 
> and
> whether the you think the library should be accepted into Boost. 
> Further
> guidelines for writing reviews can be found on the website at:
>
> http://www.boost.org/more/formal_review_process.htm#Comments

> What is your evaluation of the design?

The current design offers the flexibility needed for all serialization 
tasks I can think of and makes it possible to implement serialization 
without any performance penalty.

> What is your evaluation of the implementation?

I did not look at many details of the implementation but have a few 
comments:

1. there are still a few "dangerous" code portions, such as

     load_binary(const_cast<char *>(s.data()), l);

in line 89 of file basic_binary_iprimitive.ipp . It is stated in a 
comment that this might be unsafe, but it is probably OK on all current 
implementations of the standard.

2. the comments in the source files are at several places out of sync 
with the implementation, they sometimes still refer to old designed of 
the library. A thorough check of all files is needed.

3. looking at the implementation after reading the documentation is 
confusing at some places because of tricks used to work around compiler 
deficiencies. For example, the documentation discusses load() and 
save() functions but the implementations use load_override() and 
save_override() functions. I would prefer to have the implementation be 
consistent with the documentation and the workarounds included only in 
#if .... #endif guards.

> What is your evaluation of the documentation?

The documentation is OK for using the library, but more and better 
documentation is needed for:

1. implementing new archive types: there, as Robert is aware, the 
example in the documentation is incorrect. A more extended 
documentation would be very useful.

2. Better documentation for the classes used to track classes, class 
version, objects, etc.. For example, in newarchive.html there is a 
table listing some types (class_id_optional_type is listed twice???) 
but I am missing
   a) more information about the purpose of these types
   b) I found some additional types such as class_name_type, which is 
used in the implementation of some archives but not documented.

3. Documentation for the workarounds used to placate deficient 
compilers (e.g. save_override, ...) is missing

4. Documentation of archive formats, especially what class and object 
information is stored. I want to be able to predict, from reading the 
documentation, what exactly will be written to the archive and in which 
order. This is essential when exchanging data with an application that 
does not use this library.

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

In my opinion this will be among the most useful and most widely Boost 
libraries.

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

I tried using the library on MacOS X 10.3 using Apple's version of the 
g++ 3.3 compiler. I encountered two problems.

1. archive /wcslen.hpp does not compile because for some unknown reason 
boost/config.hpp defines BOOST_NO_CWCHAR, although the <cwchar> header 
exists. Is there a deep reason for this configuration option on MacOS X 
or is this a bug in the configuration?

2. trying to compile text_wiarchive.cpp and xml_wiarchive.cpp  I get 
error messages such as

boost/archive/iterators/remove_whitespace.hpp:146: error: `
    boost::archive::iterators::filter_iterator<Predicate, 
Base>::m_predicate'
    has incomplete type

I did not investigate this further at this time because of lack of 
time. and because I do not currently need the wide stream archive 
types.

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

Being busy and traveling from Switzerland to California has not allowed 
me time for an in-depth study of all aspects. I focused compiling the 
library under MacOS X, writing short example programs to studdy the 
archive formats with, and implementing a new archive type.

> Are you knowledgeable about the problem domain?

I have use serialization in a portable binary format on a daily basis 
for a decade.

To summarize, I vote for accepting the library into Boost but would 
like to see the documentation improved.

Matthias