[boost] Re: Serialization Formal Review #2

From: Andreas Huber (ah2003_at_[hidden])
Date: 2004-04-26 04:40:20

Robert Ramey wrote:
> Message: 11
> Andreas Huber wrote:
> 
>> 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.
> 
> I did try this in my environment VC 7.1 and it failed to work.  Does
> anyone want to assert that it should or shouldn't work?

I'm unsure about VC7.1. It is supposed to work with conformant compilers as 
detailed here:

http://www.boost.org/libs/smart_ptr/intrusive_ptr.html#Introduction

However, the situation is different there as the called function only accepts 
exactly one parameter. IIRC, if ADL works correctly it looks in all namespaces 
of all function arguments for a match. I'll see whether I find this rule in 
the standard.

>> 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.
> 
> I used save/load consistently specifically to distinguish from
> read/write which seemed to me too suggestive of persistence, files
> and streams. 

Streams have nothing to do with persistence and files, do they? As I mentioned 
below an archive *is* in some way a stream, so read/write seems more 
appropriate, especially since standard stream classes also use this 
terminology. Save/load seems *much* closer to persistence and files than 
read/write is.

>> 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).
> 
> Archive is perhaps a little too suggestive. I borrowed from MFC. 
> Making changes would have a huge ripple effect through code, file
> names, namepace names and documentation. I don't find any proposals
> for alternate naming sufficiently compelling to justify this.

Global search/replace is your friend ;-). Seriously, if I'm the only one with 
these concerns I rest my case now and will happily use your library as is.

>> 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).
> 
> Correct.
> 
>> - 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."
> 
> Correct.
> 
>> The second sentence seems to contradict the first one in that it
>> does not specify that the sequence of saving is important
> 
> I see no conflict here

Yeah, this was a misunderstanding on my part. Sorry for the noise!

> 
>> (I assume that pointer_conflict exception is not thrown if we save
>>  normally first and then through a pointer).
> 
> Correct
> 
>> Moreover, I don't see how the library could possibly
>> achieve the "anywhere in the program" bit.
> 
> It is tricky and it does work.  I works by instantitiating a static
> variable.  Static variables are constructed before main() is invoked.
> These static variables register themselves in global table when
> constructed.  So even though a template is used "anywhere in the
> program" the entry in the global table exists before execution
> starts. I'm reluctant to include such information in the manual.

I agree that such information should probably not be in the tutorial but it 
could certainly be in the reference.

> 
>> 5. Object tracking 2: It should not be possible to serialize pointers
>> referencing objects of types that have track_never tracking type.
> 
> LOL - tell that to library users.  There are lots of things that
> users are convinced they have to do that I don't think are a good
> idea.  As a library writer I'm not in position to enforce my ideas
> how users should use the library.  On this particular point I would
> be very suspicious about such a usage but as practical matter I just
> have to include a warning and move on. 
> 
> 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. 
> 
> I don't understand this.

My point is that if a user absolutely needs to save something pointed to by an 
untracked pointer then she can always do so by dereferencing the pointer first 
(I highly doubt that this is necessary very often).

int i = 5;
int * pI = &i;
ar & *pI; // This works
ar & pI; // This doesn't (exception is thrown)

It makes the user much more aware what really happens if he saves something 
pointed to by an untracked pointer.
This way it is also absolutely clear what the users responsibility is when 
such a pointer is *loaded*. Since dereferencing a pointer that has an 
undefined value is, um, undefined behavior the user must set the pointer 
*before* dereferencing it and loading the value from the archive.

>> 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.
> 
> A main function of using a trait is to avoid instantiation of code
> that 
> won't ever be used. If I understand your proposal we would have to
> instantiate tracking code even if it isn't used.

Couldn't track() instantiate the code?

> But my main objection thing about this proposal is that it introduces
> a runtime modality into the type traits.

I agree that it's a bit of a hack but the current way of doing things 
(wrapping all untracked types into new types) doesn't sound very appealing 
either. It forces me to change my design. Only rarely but I still have to 
change it. track() would allow me to save anything *as is*.

> This sound like it would
> very hard 
> to debug and verify.  I see lots of difficulties and very little
> advantage 
> to trying to add this.

Since I haven't had the time to study the implementation I can only guess & 
assume but I don't see how this is hard to debug and verify. As mentioned 
under 5. above, it should be an error to save through an untracked pointer. I 
would therefore be forced to *always* use track() whenever I want to save an 
untracked pointer directly. This makes it immediately visible in the user code.

The (IMHO big) advantage is that I don't have to pull tricks to save something 
through an untracked pointer.

> 
>> 7. I think the enum tracking_type should rather be named
>> tracking_mode as the _type suffix suggests something else than an
>> enum. 
> 
> I don't think the change is sufficiently compelling to justify the
> hassle associated with this.

Again: Global search and replace is your friend ;-). Seriously, if nobody else 
has concerns here I'm happy with it as is.

>> 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?
> 
> True.  All archives that might be used have to be included.  This is
> an artifact of using templates rather than virtual functions. 
> Templates regenerate the code for each combination of serializable
> type and archive. This results in faster code - but more of it. 
> There were strong objections raised in the first review about the
> fact it used virtual functions rather then templates - so it was
> changed to templates here. 

Ok, but IIUC this undermines the original promise that a class implementing 
serialize or safe/load can be serialized to *any* archive, even future ones 
without changing its implementation. I see that I wouldn't need to change the 
serialization *code* but I'd need to add an include to the .cpp file, right?

> 
>> 9. The library registers all kinds of stuff with static instances.
>> This precludes its use outside main().
> 
> Hmmm - that would be news to me.

Maybe I haven't stated it clearly enough: Your library cannot be used to 
save/load anything before main() is entered nor can it be used to save/load 
anything after main() has been left. This stems from the fact that code 
running before or after main() is ultimately always called from a constructor 
or destructor of some static instance (threads aside for the moment). Since 
the C++ standard does not guarantee anything about the sequence of 
construction of these instances, code saving/loading stuff could well be 
called *before* all the registering is done. OTOH, when I save something from 
a destructor of a static instance, this could be done after you have 
destructed a table containing registration information.
If I'm reading the standard correclty, then it does not currently allow us to 
work around these issues *in any way*. Therefore, this limitation should be 
mentioned in the docs.

Regards,

Andreas