[boost] Re: Serialization Library Review

From: Alberto Barbati (abarbati_at_[hidden])
Date: 2002-11-12 17:07:19

Hi David,

- What is your evaluation of the design?

I like the design very much. It's neat and far superior to other 
alternatives I evaluated in the same problem domain. I have just a few 
observations:

1) I don't like the non-intrusive way of specifying the 
version/save/load operation. The rationale for having the 
serialization<> template in the global namespace is quite weak. The 
alternative of using free functions is much better IMO. The library 
should use unqualified calls to those free functions and Koenig look-up 
would do the rest. This solution has also the benefit that 
specialization is replaced by overloading, a feature that is more easily 
supported by compilers. In this framework the 
"boost::serialization_detail" hack (which, in fact, is no more than a 
compiler hack, the natural syntax being correct to me) would probably 
not be needed.

2) A most needed addition to the design is to provide a sort of 
"registry" object. This object would present one single public method 
with the same semantic as an archive's register_type(). Then the 
archives constructors would take such a "registry" object and 
immediately invoke register_type() for each type contained there. With 
this approach, we have a better partition of responsibilties. The module 
that actually serialize objects no longer depends on the list of all 
serializable objects in the program, it needs only be given a registry 
object instance. Typically, such instance would be a singleton. How the 
registry is created and initialized is application responsibility: there 
may be a know-it-all module or the programmer may use macros a la MFC to 
let each class self-register.

- What is your evaluation of the implementation?

I did not go very deep in the implementation. However, I have again a 
few comments:

1) exception handling should be made optional. This is as easy as 
replacing each "throw" with a call to boost::throw_exception and use 
macro BOOST_NO_EXCEPTIONS to #ifdef the try/catch lines in function 
serialization<T>::load_ptr (file serialization_imp.hpp).

2) in the text_oarchive and boarchive constructors, there is a seekp(0) 
on the given stream. This is an error, IMO, and the offending lines 
should be removed. First of all there is no reason why the constructor 
should move the position in the stream: the programmer may have have all 
reasons not to like that (for example the serialization data could be 
embbedded in a larger file). Second, and most important, it is not ANSI 
conforming. In fact the standard says explicitly "If sp has not been 
obtained by a previous successful call to one of the positioning 
functions on the same stream the effects are undefined." There is at 
least one platform (Metrowerks CodeWarrior, MSL) where seekp(0) fails on 
newly-created ostringstreams, leaving the stream in "failed" state. As, 
in the current implementation, there is no status check after the 
seekp(0), any further operation on the given stream will fail as well.

3) the implementation should not assume that slist and hashed containers 
are in the std namespace. The macro BOOST_STD_EXTENSION_NAMESPACE should 
be used instead. This is required on at least one platform (CodeWarrior).

4) at least in all public headers, unused function parameters should be 
either unnamed or their names commented out, in order to avoid 
compilation warnings from pedantic compilers.

- What is your evaluation of the documentation?

The documentation is quite basic, but is a good start. The tutorial code 
should is probably outdated and should be reviewed (for example all 
friend declarations are missing the "class" keyword). In several places 
there are problems with < and > being replaced with HTML-ish &lt; and &gt;.

One note: the library, as it is, *does not* support Unicode output, as 
stated. The library supports wide streams, yes, but that does not mean 
Unicode support. In fact, the standard says explictly that the 
conversion facet codecvt<wchar_t,char,mbstate_t> used by default by wide 
streams shall "implement a degenerate conversion". For example, if 
wchar_t has 16 bits and char has 8 bits (typical situation of x86-based 
plaftforms), that means that the high byte of each wchar_t is simply 
discarded. Thus Unicode characters outside the 0-255 range will not be 
output correctly, unless an alternative conversion facet is imbued in 
the stream before passing it to the library. (Shameless plug: that's why 
I've tried to propose alternate codecvt facets for Unicode... but no one 
seems to realize the real issue.)

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

The potential usefulness of library is huge. However the lack of a 
centralized registration component for polymorphic types restricts its 
applicatibility.

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

I tried little demo programs with two different compilers. That was not 
a deep test with live data, although I plan to do it in the next few 
days. I tested both polymorphic and non-polymorphic types, but no templates.

On VC++ 7.0 (.NET) it all went good at the first try. However the 
restriction, cited in the documentation, of having to add /OPT:NOREF to 
the linker options is too hard to swallow. In a typical setting, this 
can increase the executable size by up to 100% or even more, as such is 
the typical size of unreferenced data. This problem will have to be 
addressed explicitly.

On Metrowerks CodeWarrior I had a few problems. A few of them I already 
described above. Another one come from the fact that the library uses 
(correctly) the trait is_base_and_derived in the implementation of 
template class base_object. is_base_and_derived depends on 
is_convertible that is broken on CodeWarrior. I had to comment out the 
static assert line to let the program compile.

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

I read the documentation quickly, then write those test programs. I 
traced the execution in the debugger to discover the seekp(0) issue and 
to have glance at the library internals, but that does not account for 
in-depth study.

- Are you knowledgeable about the problem domain?

I can say that I am a bit knowledgeable. I worked on (and know all 
quirks of) the MFC implementation and tried myself at least two 
different approaches, although they were limited in the scope of the 
respective applications and not general-purpose components.

- Do you think the library should be accepted as a Boost library?

My opinion is that the library should not be accepted as it is, but has 
huge potential. There is indeed little space for improvements, but a few 
features, such as the registration of polymorphic types, should 
defintely be addressed before prime time.

Alberto Barbati