[boost] [Serialization] Formal Review

From: Matthew Vogt (mvogt_at_[hidden])
Date: 2004-04-19 02:36:17

Formal review points:

* What is your evaluation of the design?

The design is rather complex - it certainly allows great flexibility, at the
cost of ready comprehensibility.  I realise that the complexity has accrued
as a result of the library's extensive 'generalisation' phase; it would
be nice to see a brief 'design and evolution' section in the documents.

It's a little hard to comment on the design without having participated in
the first review; I hope other reviewers can address this.

* What is your evaluation of the implementation?

The implementation seems quite clean, and well ordered. I think the comments
in the code need a review pass, since they are occasionally out-of-sync
with the code they comment upon.  Also, it would be nice to see a bit more
commentary on the purpose of the various classes that make up the
implementation, which aren't covered by the documentation.

The library includes part of the code as '.ipp' files, for inclusion
into user '.cpp' files. This is not a common practise in boost libraries;
perhaps it needs to be documented somewhere in the Boost:More Information
section.

* What is your evaluation of the documentation?

The documentation is excellent. The layout is ideal, progressing through
tutorial, reference and implementation sections.  I think the documentation
addresses extending the library with serialization of external types better 
than it addresses extending the library with new archives (although, this is
probably a biased view :) )

I think the 'Advice' section is redundant; these points should be moved to 
the relevant reference section instead.

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

I think the library is potentially of very wide applicability, due largely to
its extensibility in the separate domains of types and archives.

* Did you try to use the library?  With what compiler?

Yes, with GCC 3.2.2, without problems.

* How much effort did you put into your evaluation? 

I read the documentation closely, investigated portions of the implementation,
and tried to extend the library with types and archives.

* Are you knowledgeable about the problem domain?

Yes, and no; the library's applicability is broad, so few people will have
extensive knowledge of the whole problem domain.  I am more familiar with
serialization for marshalling than for persistence, or for interoperability.

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

Yes.  I can't think of any major issues that would force further re-design 
in the library, although new implementation detail issues are surely likely; 
I think it should be accepted now.

Addressing Jeff's questions:

1) This is a large, complex, and important library. Please don't wait till the
end of the review period to start your review -- there is no additional room
in the review schedule to extend the review period.

This is a brief review; please ask if you would like detail on any response.

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.

As mentioned earlier, I think that the documentation covers the handling of
types better than extension with archives.
The section 'New Archives' needs to address the difference between overriding
'save' and 'save_override', and 'operator>>'.
Also, discussion of the archive's preamble, and how to disable or modify it
seems to be necessary.

That said, the library seems not to limit anything an archive implementor may
want to do. My only remaining concern is regarding controlling access to
indiviual overloads of 'save' or 'load'.  This seems to break down, due to
the fact that all accesses are from the 'access' class.

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

Not me.

4) Finally, I would like reviewers to indicate parts of the library that 
might become standalone boost libraries.  While this will have little bearing
on the acceptance / rejection of the library it might allow a reduction in 
the size of the library.

Some sections of code are definitely candidates for extraction from the 
serialization library, largely identified by Robert in the 'Miscellaneous' 
section of the documentation.

The strong typedef and state_saver utilities could be extracted, perhaps to the
utility library.
Perhaps the extended type info infrastructure is useful outside the library.
The dataflow iterators are very interesting, but I'm not sure how they interact
with the iterator adaptors library, or with sandbox views library.  
The unicode conversion code is certainly useful, but I imagine it might be
better to leave it as an implementation detail of serialization, until someone
volunteers to investigate a complete solution for this issue.  The code
dealing with XML formatting and escaping is in a similar situation.

Thanks Robert, for your efforts

Matt