[boost] Re: Serialization Formal Review #2

From: Andreas Huber (ah2003_at_[hidden])
Date: 2004-04-25 18:36:41

Here's my review:

***** What is your evaluation of the design? *****

The overall design seems sound. However, apart from what other reviewers
mentioned I have a few additional suggestions and nitpicks (I did not have
the time to follow all the discussions, so some points may have been brough
up already):

1. On compilers supporting ADL, it seems the user-supplied serialization
function for non-intrusive serialization

template<class Archive>
void serialize(Archive & ar, gps_position & g, const unsigned int version);

could also be put into the namespace of the type that it serializes. If this
is correct it should be mentioned in the tutorial & reference. If not, there
should be a rationale why not.

2. save/load naming:
All functions that have to do with reading from an archive are named load*,
all functions that have to do with writing to an archive are named save*. To
me (non-native speaker) these names are generally associated with
persistence only. Since the library is more general (one could use it for
network communication, etc.) load/save are a bit missleading. I'd rather see
read/write or other names that more clearly communicate what the functions
are doing.

3. archive naming:
The word archive also seems to be associated with persistence. I think
serializer would much better describe the functionality. Another option I
could live with is serialization_stream (I slightly disagree with the
assertion in archives.html that an archive as modelled by this library is
not a stream. It might not be what C++ folks traditionally understand a
stream is but to me it definitely is a more general stream, i.e. a stream of
objects/things/whatever).

4. Object tracking 1: Seeming contradiction
I've had difficulty determining how exactly object tracking works. I believe
the associated docs are slightly contradictory:
- Under exceptions.html#pointer_conflict I read that the pointer_conflict
exception is thrown when we save a class member through a pointer first and
then "normally" (i.e. through a reference).
- traits.html#tracking says "... That is[,] addresses of serialized objects
are tracked if and only if an object of the same type is anywhere in the
program serialized through a pointer."
The second sentence seems to contradict the first one in that it does not
specify that the sequence of saving is important (I assume that
pointer_conflict exception is not thrown if we save normally first and then
through a pointer). Moreover, I don't see how the library could possibly
achieve the "anywhere in the program" bit.

5. Object tracking 2: It should not be possible to serialize pointers
referencing objects of types that have track_never tracking type.
special.html#objecttracking says:
<quote>
By definition, data types designated primitive by Implementation Level class
serialization trait are never tracked. If it is desired to track a shared
primitive object through a pointer (e.g. a long used as a reference count),
It should be wrapped in a class/struct so that it is an identifiable type.
The alternative of changing the implementation level of a long would affect
all longs serialized in the whole program - probably not what one would
intend.
</quote>
I think serializing untracked pointers almost never makes sense. If it does
anyway the pointed-to object can always be saved by dereferencing the
pointer. Such an error should probably be signalled with an exception.

6. Object tracking 3: One should be able to activate tracking on the fly
Rarely it makes sense to save even primitive types in tracked mode (e.g. the
example quoted in 6.). That's why I think setting the tracking type per type
is not flexible enough. It should be possible to override the default
somehow. I could imagine to save such pointers as follows:

class T
{
  ...
  int i;
  int * pI; // always points to i
};

template<class Archive >
void T::save(Archive &ar) const
{
    ar << track( i ); // save the int here, keep track of the pointer
    ar << track( pI ); // only save a reference to the int
}

Since it is an error to save an untracked type by pointer even the second
line needs the track( ) call.

7. I think the enum tracking_type should rather be named tracking_mode as
the _type suffix suggests something else than an enum.

8. I don't understand how function instantiation as described under
special.html#instantiation works. The description seems to indicate that all
possible archive classes need to be included in the files of all types that
are exported. Is that true?

9. The library registers all kinds of stuff with static instances. This
precludes its use outside main(). This and other limitations should be
stated explicitly in the rationale.

***** What is your evaluation of the implementation? *****

I did not have a look at the implementation.

***** What is your evaluation of the documentation? *****

10. The tutorial is good. A few more links to advanced stuff could make it
even easier to find your way around the library, e.g. how to split free
serialization functions.
11. As other reviewers have already pointed out, the reference needs some
serious work (this is not to say that Robert did a bad job, it's just not
there yet). Some stuff is explained in "tutorial-language", i.e. too
informal what left me wondering about the details (I can give examples if
necessary). Moreover, the layout should be in C++ standard-style and include
all the concepts, classes, functions and macros provided.

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

Very useful.

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

No, I did not try to use the library.

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

About 6 hours reading the docs and writing this review.

***** Are you knowledgeable about the problem domain? *****

Sort of. 5 years ago I tried to write a library covering a small subset of
what this library does. I eventually abadoned it because of compiler
problems and lack of time.

***** Do you think the library should be accepted as a Boost library?  Be
sure to say this explicitly so that your other comments don't obscure your
overall opinion. *****

Yes. Given that the points mentioned above are resolved I vote to accept the
library into boost.

A big thanks to Robert for all this excellent work!

Andreas