Re:[boost] Re: Serialization Formal Review #2

From: Robert Ramey (ramey_at_[hidden])
Date: 2004-04-25 23:58:34

Message: 13
Fredrik Blomqvist

> * What is your evaluation of the implementation?
> As I mentioned in my previous review I would like to see it handle most of
> the STL and core boost components "out of the box". 

I believe all STL is handled.  Serialization of other boost components will
have to be the responsibility of their authors or others.

> In particular I think the smart_ptrs (including auto_ptr) should have
> core-support from the serializer, even special cased if necessary (similar
> to boost::bind etc). 

I believe the current library is capable of implementing serialization for
all boost components.  Much effort was expended to that the library would
not have to have special code for any serialization or archive type. In
fact, my interest is just the opposite. I would like to see a significant
part of the library moved outside.  Anyone who wants to volunteer to take
responsibility for any of the component please let us know.  The library
would be improved by making it smaller.

> The code to serialize std::auto_ptr could easily be copied to the official
> directory for example.

There has been some discussion of where the serialization code for other
boost libraries should reside. I don't have strong feelings about this other
than I would prefer that it reside outside the serialization library itself.
The only exception I would make is for the STL serialization which has no
where else in boost to reside and it has a "special" status as and official
C++ library.

> Regarding shared_ & weak_ptr. If the implementation path Robert outlines
> in the documentation is to be used I guess an official patch to
> shared_count is necessary. Would be interesting to hear what the smart 
> pointer experts think.

I agree - the silence is deafining.  I know there are applications that use
my smart pointer serialization code right now.  I implemented and tested it
but I don't really know for a fact that it addresses all aspects that need
to be addressed such as exception safety, threading, and who knows what
else.

> Otherwise, I still think the solution I outlined in my previous review
> that doesn't require modifying shared_ptr could be considered. Downside
> being that it most likely adds a special-cased codepath to the serializer,
> but as I said I think it would be worth it.

I would hope that the developer's maintaining shared pointer will address
this.  putting special case code inside the serialization library would be a
serious mistake.  Between code, tests, and documentation, its approaching
30,000 lines.  

> The code is nicely written altough complex. Highly nested template code
> and lots of mpl. All compile-time checks and further factorizations are
> valuable.

> Nitpick: serialization/scoped_ptr.hpp seems to have a typo. An extra
> #pragma once is present outside the #ifdef (_MSC..) block. It also
> includes serialization/pfto.hpp but doesn't seem to use it.

Thanks - I love picking these nits.  Its easy and satisfying - really.

> * Parts of the library that might become standalone boost libraries
> The library already provides good separation of components. tatic_warning,
> strong typedef, pfto, stack_allocate etc are all given candidates that
> would just need better documentation and (fast-track?) reviews.

A little bit more is going to be required.  They've been tested in the
context of serialization and that's OK.  I think they need to be more
polished and that's a little more work than it first appears. Also a
separate review would probably fine tune their interface, implementation and
capability. 

* What is your evaluation of the documentation?
> I would also like to see an improved reference documentation that clearly
> states the requirements on the types/concepts involved.

Hmmm - that seems pretty unanimous - pretty much a first for boost as far as
I know.

> I support the separation of documentation (and implementation) of other
> boost components that like to support serialization. A simple index with
> links would suffice I think but most importantly a _consistent_ structure
> should be agreed upon which new components can follow.

I followed the boost document on how to write documentation.  The only
wrinkle I added was the right hand index/contents window.  I'm pretty
pleased with this.

I would appreciate VC7.1 project files for the examples and tests though.

> One of my tests added support for the STL container adaptors std::stack,
> queue and priority_queue. I think direct support for them could be
> convinient since they hide the native containers. I attach my
> implementation. Feel free to comment

I took a short look. The name stack_hack sort of begs some sort of comment.
The main thing is that any component of this type has got to be submitted
with a test. 

Thanks for taking the time to study the library and submit a review.

Robert Ramey