Re: [boost] [serialization] Regressions?

From: David Abrahams (dave_at_[hidden])
Date: 2005-06-25 07:46:20

"Robert Ramey" <ramey_at_[hidden]> writes:

> David Abrahams wrote:
>> "Jeff Garland" <jeff_at_[hidden]> writes:
>>
>>> On Fri, 24 Jun 2005 09:19:05 -0400, David Abrahams wrote
>>
>> Yeah, with all due respect to the author -- who has designed a library
>> that's by all accounts very satisfying -- this design choice is just
>> all wrong.  It doesn't detect what it purports to, and gives plenty of
>> false positives.  Because it's a compile-time check people will get
>> used to doing what is required to subvert it.
>
> This is very speculative.  

No, it is based on well-earned experience and observation.  This is
what happened in Java with exception specifications and it's what
happens when people get compiler warnings that are often wrong.

> Some people may realize the error of their ways and correct their
> practices - who can say?

Most people with experience in writing C++ (and probably a few others)
will realize that their ways are not fundamentally erroneous, as
several of the more experienced people on this list have.

It's normal that anything you can do with a const lvalue can also be
done with a non-const one.

>> It's well known that error reports that are commonly wrong are
>> worse than no report at all.
>
> First I don't think the error report is commonly wrong.  Second, it will be
> almost impossible for the person who commits such an error to find it
> if/when it occurs.

Great, then use a detection mechanism that will actually catch the
error.  Hashing does that.  In fact it will catch instances that
refusing to serialize non-const lvalues will not.  The for loop
you showed in an earlier post is a prime example.

>>> I never stopped caring, I just got tired. Since the moment this
>>> change went into the library and blew up date-time tests and some of
>>> my other programs that were working perfectly I was unhappy.  But
>>> neither Vladimir or myself have been able to convince Robert that
>>> this change is ill advised
>>
>> I've noticed a similar dynamic with a few other Boost libraries
>> recently.
>
> There might be an explanation for that.  I get feedback from people with
> questions about using the library.  Much of it is via private email.  This
> is quite a different group than those that inhabit this list.  On the list I
> get a lot of concerns about about usages of the library in ways I never
> imagined.  Also many of the commenters on the list have strong opinions
> driven by their particular application.  I feel I have to be concerned about
> correctness, transparency, efficiency, portability, long comple times, etc.
> Many times someone is disappointed I don't include this or that change
> because it conflicts with one of these aspects which is uninportant to them.
> So everyone is disappointed at least to some extent.  This is the root cause
> that larger libaries that lots of people have an interest in
> (e.g.units/dimensions) can't make progress in boost.  (Serialization would
> fall into the category except for my particular personality features.)

Please don't trivialize the concerns of those experienced library
developers who are upset about this.  This is not about personal
disappointment and desires; it's a fundamentally deeper issue about
consistency with the way users expect C++ to work.

> What is true is that the state of an object should not be changed durring
> the process of serialization.  That is the statement made by save(Archive
> &ar, const T & t).

No, the statement made is that *save* itself promises not to change t.

> The problem you had was the same as mine when writing the tests.  The
> typical code was
>
> A a, a1;
>
> text_archive oa(..)
> oa << a;
> ...
> text_iarchive ia(..)
> oa >> a1
>
> BOOST_CHECK(a == a1)
>
> Of course this was easy to fix (just make use const A a1) but other cases
> required a little more work - but never very much.
>
> But I argue - as Joaquin did.  That the above situation is not at
> all typical and that in typically the situation almost never comes
> up.  Of course that is speculative on my part.  But so far I've only
> gotten objections from library testers.  

I am not even testing a library.  I am just looking at this from the
point of view of language and library consistency.  I think this
particular design choice sets a very bad precedent, and people look to
Boost as an example of good C++ coding style.

> Vladimir came up with a bunch of cases but they seemed very atypical
> to me.  The were at best usages of the library in ways that have
> never occurred to me so it was hard for me to be convinced.
>
> That's why I say I believe its been blown waaaay out of proportion.

I think I care about this for reasons you haven't considered.  Yes,
it's "easy to work around," but it doesn't make sense.  That's more
important to me than the fact that it's a little inconvenient.

>>>  The following template function will allow for this (and is
>>>  used in the date_time tests). At this time no special steps are
>>>  necessary to read from an archive.
>>>
>>>       template<class archive_type, class temporal_type>
>>>       void save_to(archive_type& ar, const temporal_type& tt)
>>>       {
>>>         ar << tt;
>>>       }
>>>
>>> Feels/looks like an ugly workaround to me...
>
> typically that's the way it gets used anyway.  Which is exactly my point.
>
>> Yah.  Just another pointless hoop to jump through.
>
> Yeah - just like the whole "const" business in the first place?

No.  The "const" business catches real problems and provides a
framework for reasoning about code... as long as it's used
consistently.  Making an operation legal only for const objects is
inconsistent with the way "const" works in C++.

> For what its worth there are a couple of incidental aspects of this
> situation that might be interesting.
>
> a) This whole thing is implemented by one STATIC_ASSERT in oserializer.hpp
> which can be commented out
> b) I considered using STATIC_WARNING but I could never really quite
> STATIC_WARNING to function totally to my taste.
> c) After I ran this with my own tests, and being forced to think about each
> instance, I concluded that my code was more fragil than I had thought and
> became more convinced that it was a good idea.

If you think some checks are needed, hashing provides the right ones.

> d) Also having to find my own errors in serialization of stl containers
> convinced me it was a good idea.

Can you show all the code you wrote that stopped compiling after the
introduction of this check?  Can you also describe the thought process
you used to decide which examples were erroneous and which should be
silenced by the addition of a const?

> I would like to wait and see how this works for a while.  So far I've heard
> from
>
> myself - library writer
> Jeff - wrote serialization test for date/time
> Joaquin - wrote serialization for mult-index - a tricky job
> Vladimir - using in in real code.
> Dave - not used the library in any way
>
> As far as I'm concerned we really have only one data pont (Vladimir) which
> really addresses what the discussion is about.
> So lets relax a bit and let some more experience trickle in for a while and
> see what happens.

I'd like to convince you not to let this one out into the world,
though :)

-- 
Dave Abrahams
Boost Consulting
www.boost-consulting.com