[boost] Re: [serialization] Regressions?

From: Robert Ramey (ramey_at_[hidden])
Date: 2005-05-11 01:06:47

Vladimir Prus wrote:
> Robert Ramey wrote:
>
>>>> instantiated from this code of mine:
>>>>
>>>>         ofstream ofs(av[2]);
>>>>         boost::archive::binary_oarchive oa(ofs);
>>>>         oa << *data_builder.finish();
>>>>
>>
>> This code should be considered erroneous.  The documentation in 1.32
>> addressed this but unfortunately the enforcement was lost in the
>> shuffle.
>
> Excuse me, but I refuse to accept that this code is erroneous. As
> I've said, this is my existing design -- the 'finish' function
> returns non-const pointer. The fact that somewhere else, I serialize
> the result of 'finish'
> has nothing to do with 'finish' return type, and I should not be
> forced to change my existing interface just because I use
> serialization somewhere.

OK let me rephrase.  This code conflicts with the description of the
description of the functioning of the operator << as described in the
documentation.

>
>> The intention is to trap the saving of tracked non-const objects.
>> This is to prevent users from doing something like
>> For(...
>>     A a;
>>     ...
>>     ar << a; // will save the address of a - which is on the stack
>>
>> If a is tracked here, instances of a after the first will be stored
>> only as reference ids. When the data is restored, all the as will be
>> the same. Not what the programmer intended - and a bear to find.
>> This is really the save counterpart to the load situation which
>> required the implementation of reset_object_address.
>
> So, you're saing that "const" == "no allocated on stack"? I don't see
> why this statement is true. I can just as well do this:
>
>   void foo(const A& a)
>   {
>          ar << a;
>   }
>
> and circumvent your protection.

The intention is that const indicates that the process of serialization will
not change the object being serialized.  Serializing and object that cannot
easily be passed as a const is quite possibly an error.  Of course, the
contrary is not necessarily true. That isan object can be passed as  const
referene and still be modified during the process of serialization.  So this
is not bullet proof - but I believe it is very helpful.

> Further, how often is it that non-pointer object is tracked? I think it's
rare case, while saving a
> pointer is a
> common case, and making common case inconvenient for the sake of
> non-common case does not seem right.

It can happen more often than you might think.  Its easy to serialize an
object and sometime later serialize a pointer to the same object.  In order
to avoid creating duplicates all the instances of an object - not just the
pointers, have to be tracked if a pointer to that class of object is used
even once.  (As an aside, the serialization libary detects the situation
where an object is never serialized through a pointer and suppresses
tracking in this case).

>> Note that the documentation suggests that he above be reformulated as
>> For(...
>>     ar << a[i]; //
>>
>> Enforcing const-ness also has the effect of preventing serialization
>> from altering the state of the object being serialized - another
>> almost impossible to find bug.
>

> Can you explain:
> 1. How this bug can happen

Its very easy to write
for(...{
    X x = *it; // create a copy of
    ar << x
}

all the x's are at the same address so if they happen to be tracked because
a pointer to some X in serialized somewhere in the program, then the
subsequent copies would be supprsed by tracking.  With the above formulation
this cannot happen. This error is detected a compile time - very convenient.
Of course the following would work as well

for(...{
    const & X x = *it; // save a const reference to the orgiinal
    ar << x
}

while the following would create an error that would go undetected.

for(...{
    const X x = *it; // create a copy of
    ar << x
}

> 2. Why the "watch" command on gdb is not a reliable way to catch such
> bugs?

I'm not sure what the gdb watch command does but I'm sure it doesn't detect
compile time errors.

>> Also, I had to tweak a number of my tests and demos to make them
>> work with this new rule. However, the tweaking was not difficult. If
>> altering one's code to conform this rule isn't easy, one should make
>> an effort to understand why. Its possible that a very subtle and
>> hard to understand bug might be being introduced.
>
> Are you saying that my code is buggy?

I'm saying you've passed up an opportunity to permit the compiler to flag
someting that could be an error.

>>>>         ofstream ofs(av[2]);
>>>>         boost::archive::binary_oarchive oa(ofs);
>>>>         oa << *data_builder.finish();

Looking at the above, I have to concede I never envisioned it being used
this way.  I would need more context to really comment intelligently on it.
I will say that I alwas envisioned the interface as being used in a
declarative style.  That is, I envisioned the the serialization declarations
ar & x or ar << x as a shorthand for "this member is persistent".   I'm not
saying that you're doing anything wrong, its just not what I expected.

>>>> Where 'finish' returns auto_ptr<Data>. It's looks like
>>>> serialization checks if the serialized type is 'const' and if not,
>>>> complains.

correct

>>>> Basically, it's some heuritic to prevent saving
>>>> on-stack object

That's one case I would like trap.

>>>> (though I don't understand why it would work at al).

I don't understand this.

>>>> I find this a bit too much . I have no reason whatsoever to make
>>>> 'finish' return auto_ptr<const Data>. And writing
>>>>
>>>>         oa << const_cast<const Data&>(*data_builder.finish());

When finish returns something that is not a const it suggests that its
returning a pointer to a mutable object.  The serialization library presumes
that objects don't change in the course of serialization.  So passing a
non-const conflicts with one of the assumptions made in the implementation
of the library.

>> That is why the STATIC_ASSERT only trips when
>> tracking is enabled for the corresponding datatype.
>>>> looks very strange. And modifying all places where I get this error
>>>> is not nice too. So, can this static assert be removed?

Its there for a purpose.  How many places do you get this error? Returning
an auto_ptr to the top of the stack is interesting to me.  doesn't that
destroy the one in the original location?  If its a temporary you could just
as well return an auto_ptr to a const T.  But really without more context
its hard for me to commment.

>> As I said, if its in a lot of places one should think about this.
>> If this is truely intolerable and you don't mind driving without
>> seat belts, use the & operator instead.
>
> This is inferiour solution, because operator<< is more logical.

They're both arbitrary.

> Ok,
> 1. Why std::string is 'tracked type'?

so that storage space isn't wasted storing repeated strings

> 2. How do you suggest me to fix the above?

I did suggest using & instead.  But I'm curious to see more context.  I'm
curious to see more.

Personally, I believe const-ness is under appreciated and is helpful in
catching bugs at compile time.  In the future as more mult-threading is
used, I think it will be even more important.  Much effort was invested in
using const (and asserts) to detect bugs at compile time.  I can't catch
them all but I catch what I can and I believe that it has made the library
much easier to use.

Robert Ramey