[boost] Re: [serialization] Regressions?

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

Vladimir Prus wrote:
>> Hi Robert,
>> I've just updated by Boost tree and run in a number of compile
>> errors.
>>
>> One is pretty easy -- I've committed the attached trivial patch to
>> split_member.hpp.
>>
>> Another is this:
>>
>> ....boost/boost/archive/detail/oserializer.hpp:551: error: incomplete
>>    type `boost::STATIC_ASSERTION_FAILURE<false>' does not have
>> member `value'
>>
>> 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.

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.

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. Remember that when a tracked object is saved more
than once, only the first time is the data saved. If the object can be
changed during serialization, we have a problem.

Having said that - the & operator doesn't do the const checking. Doing so
inhibits its usage.
Also, in spite of much effort, I was unable to make the const checking
function to my taste when objects are wrapped in an nvp wrapper.

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.

I hope this explains my reasoning.

>> Where 'finish' returns auto_ptr<Data>. It's looks like serialization
>> checks if the serialized type is 'const' and if not, complains.
>> Basically, it's some heuritic to prevent saving on-stack object
>> (though I don't understand why it would work at al).
>>
>> 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());
>>

This would "fix" it but might not be necessary.  I envisioned that the
operator << would usually be used in a function templateof the following
signature

save(Archive & ar, const T &t ,...

so that normally the issue wouldn't arise.  If it does arise, its sort a
warning to really think about what the code is doing. If the return value of
data_builde.finish() can be changed during the serialization process, one is
going to have a problem.  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?

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.

>>
>> Then, I get ambiguity in iserializer.hpp, in this code:
>>
>> #ifndef BOOST_NO_FUNCTION_TEMPLATE_ORDERING
>> template<class Archive, class T>
>> inline void load(Archive &ar, const serialization::nvp<T> &t){
>>     load(ar, const_cast<serialization::nvp<T> &>(t));
>> }
>>
>> For some reason, both boost::archive::load and some other 'load' in
>> 'boost' namespace (part of earlier serialization lib) that I still
>> use are considered overload candidates. Adding explicit
>> boost::archive:: fixes this. See attached patch.
>>
>> Then I get error at this code:
>>
>>     ar << boost::lexical_cast<std::string>(*this);
>>
>> The error message is:
>>
>> error: no match for 'operator<<' in 'ar <<
>>    boost::lexical_cast(Source) [with Target = std::basic_string<char,
>>    std::char_traits<char>, std::allocator<char> >, Source =
>>    lvk::nm_model::BlockFormula]()'
>> /space/NM/boost/boost/archive/detail/interface_oarchive.hpp:75:
>> error: candidates
>>    are: Archive&
>>
>>
>>
>>
>> boost::archive::detail::interface_oarchive<Archive>::operator<<(T&)
>> [with T = std::basic_string<char, std::char_traits<char>,
>> std::allocator<char> >, Archive = boost::archive::binary_oarchive]
>> ...unrelated operator<< definitions snipped...
>>
>> Apparently, passing rvalue to "T&" does not work. Yes another
>> attached patch fixes this issue.

This is the same issue as before.  Passing a non-const tracked type to the
archive << operator.

Robert Ramey