[serialization] rvalues and const-correctness?
Consider: struct X { int f(); template <class Archive> void save(Archive& ar, unsigned) { ar << f(); // can't bind non-const reference to temporary } BOOST_SERIALIZATION_SPLIT_MEMBER() }; This error seems silly to me. Couldn't we add this overload to the archives, or even simply replace the existing operator with this one? template<class T> Archive & operator<<(T const & t){ // ^^^^^------------- Note this->This()->save_override(t, 0); return * this->This(); } TIA, -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
Consider:
struct X { int f();
template <class Archive> void save(Archive& ar, unsigned) { ar << f(); // can't bind non-const reference to temporary }
BOOST_SERIALIZATION_SPLIT_MEMBER() };
This error seems silly to me.
Hmmm - looks like a good idea to me. If f() is a non const function, then calling it while saving data could change the state of the data being saved. The library tracks repeated saving of objects and on subsequent saves would only save a handle to the original one. This saves time, space and handles multiple pointers to a single instance. If the process of saving can change the data, then all this breaks down. (not to mention saving - without locking - from multiple threads - which is now permited by the library. If f() can't be made "const" and is used in this way, its likely that this would create an archive which would not load correctly. Finding the source of this error, and correcting its effects would seem to be a nightmare to me. Couldn't we add this overload to the
archives, or even simply replace the existing operator with this one?
template<class T> Archive & operator<<(T const & t){ // ^^^^^------------- Note this->This()->save_override(t, 0); return * this->This(); }
Aaaa - I would have to spend some time to look into that. Robert Ramey
Robert Ramey skrev:
David Abrahams wrote:
Couldn't we add this overload to the
archives, or even simply replace the existing operator with this one?
template<class T> Archive & operator<<(T const & t){ // ^^^^^------------- Note this->This()->save_override(t, 0); return * this->This(); }
Aaaa - I would have to spend some time to look into that.
Isn't this the debate we have had so many times? -Thorsten
Thorsten Ottosen wrote:
Robert Ramey skrev:
David Abrahams wrote:
Couldn't we add this overload to the
archives, or even simply replace the existing operator with this one? template<class T> Archive & operator<<(T const & t){ // ^^^^^------------- Note this->This()->save_override(t, 0); return * this->This(); }
Aaaa - I would have to spend some time to look into that.
Isn't this the debate we have had so many times?
-Thorsten _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Thorsten Ottosen wrote:
Robert Ramey skrev:
David Abrahams wrote:
Couldn't we add this overload to the
archives, or even simply replace the existing operator with this one? template<class T> Archive & operator<<(T const & t){ // ^^^^^------------- Note this->This()->save_override(t, 0); return * this->This(); }
Aaaa - I would have to spend some time to look into that.
Isn't this the debate we have had so many times?
well, we havn't had it lately. But this is actually a little different since this is calling a member function inside serialization. Since save(...) is a const member funtion, the f() would have to be const. if you can't make f() const, you've got something to look into. FYI, the more resent versions of the library implement the trap where by non-const are serialized as BOOST_STATIC_WARNING so that you're program will compile and run but emit this "disclaimer" indicating that my view is that this is really a bad idea. You're free to ignore the warning whereas before you couldn't. Robert Ramey
on Thu Nov 06 2008, "Robert Ramey" <ramey-AT-rrsd.com> wrote:
Thorsten Ottosen wrote:
Robert Ramey skrev:
David Abrahams wrote:
Couldn't we add this overload to the
archives, or even simply replace the existing operator with this one? template<class T> Archive & operator<<(T const & t){ // ^^^^^------------- Note this->This()->save_override(t, 0); return * this->This(); }
Aaaa - I would have to spend some time to look into that.
Isn't this the debate we have had so many times?
well, we havn't had it lately.
But this is actually a little different since this is calling a member function inside serialization. Since save(...) is a const member funtion, the f() would have to be const. if you can't make f() const, you've got something to look into.
The member function wasn't my point at all. I just want to be able to save rvalues. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
on Thu Nov 06 2008, David Abrahams <dave-AT-boostpro.com> wrote:
on Thu Nov 06 2008, "Robert Ramey" <ramey-AT-rrsd.com> wrote:
Thorsten Ottosen wrote:
Robert Ramey skrev:
David Abrahams wrote:
Couldn't we add this overload to the
archives, or even simply replace the existing operator with this one? template<class T> Archive & operator<<(T const & t){ // ^^^^^------------- Note this->This()->save_override(t, 0); return * this->This(); }
Aaaa - I would have to spend some time to look into that.
Isn't this the debate we have had so many times?
well, we havn't had it lately.
But this is actually a little different since this is calling a member function inside serialization. Since save(...) is a const member funtion, the f() would have to be const. if you can't make f() const, you've got something to look into.
The member function wasn't my point at all. I just want to be able to save rvalues.
Have you had a chance to look into this? I've been using a patched version of the library that adds the stated overload and it is working great. Taking out the patch means that where I ought to have been able to simply write ar << f(); I am forced to write code like this: typename nasty_trait_to_compute_return_type<foo>::type x = f(); ar << x; Nasty. Please? -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
The member function wasn't my point at all. I just want to be able to save rvalues.
OK - I think I see the problem better. It would seem that you want to serialize a temporay value from the stack. Think about what this would mean in the context of pointers and object tracking. Since tracking is done via object address, and tracking is necessary in order to implement correct restoration of pointers, this can't can't be done reliably.
Have you had a chance to look into this? I've been using a patched version of the library that adds the stated overload and it is working great. Taking out the patch means that where I ought to have been able to simply write
ar << f();
what does f() return? I would expect it return a reference to a constant object of some sort. If it doesn't, then I would expect that its possible that now or sometime in the future, unloadable archives could be created. And the fact that these archives are unloadable would not be discovered until the are in fact actually loaded. Also it would be extremely difficult to determine what the problem is. Finally, it would be very difficult to fixup the archive. Of course the fact that in many particular cases it doesn't create a problem doesn't alleviate my concerns.
I am forced to write code like this:
typename nasty_trait_to_compute_return_type<foo>::type x = f(); ar << x;
If this function is called more than once, then it's possible that the address of the second object serialized will be the same as that of the first call and only an object id will be saved even if the object is in fact different, This is implemented this way to support efficient and correct serialization of pointers and incidently to save storage. In this instance, an archive will be created such that when the second object is loaded, it will be identical to the first object and this will not be correct. This error would be a major problem to find. So I would not recommend your fix above. A better way would be to restrict f() to function which return a const reference to the type returned by the current f(). If that isn't convenient, and one had nothing else to do, he could explore the implications of implementing your proposed specialization so that it traps at compile time if the object might be tracked. This would mean that something like int f() would work while something like my_object f() would likely trap unless my_object was specifically marked as no_tracking. Thinking about this just a tiny bit makes this seem attractive to me. Of course that doesn't mean much until one sets out to actually do it. Robert Ramey
Nasty.
Please?
on Sun Nov 09 2008, "Robert Ramey" <ramey-AT-rrsd.com> wrote:
David Abrahams wrote:
The member function wasn't my point at all. I just want to be able to save rvalues.
OK - I think I see the problem better. It would seem that you want to serialize a temporay value from the stack. Think about what this would mean in the context of pointers and object tracking. Since tracking is done via object address, and tracking is necessary in order to implement correct restoration of pointers, this can't can't be done reliably.
I didn't think you did object tracking for things that were serialized by value. Do you do that?
Have you had a chance to look into this? I've been using a patched version of the library that adds the stated overload and it is working great. Taking out the patch means that where I ought to have been able to simply write
ar << f();
what does f() return?
An rvalue.
I would expect it return a reference to a constant object of some sort. If it doesn't, then I would expect that its possible that now or sometime in the future, unloadable archives could be created. And the fact that these archives are unloadable would not be discovered until the are in fact actually loaded. Also it would be extremely difficult to determine what the problem is. Finally, it would be very difficult to fixup the archive.
Of course the fact that in many particular cases it doesn't create a problem doesn't alleviate my concerns.
Naturally.
I am forced to write code like this:
typename nasty_trait_to_compute_return_type<foo>::type x = f(); ar << x;
If this function is called more than once, then it's possible that the address of the second object serialized will be the same as that of the first call and only an object id will be saved even if the object is in fact different,
In my case I've turned off tracking, but yeah.
This is implemented this way to support efficient and correct serialization of pointers and incidently to save storage. In this instance, an archive will be created such that when the second object is loaded, it will be identical to the first object and this will not be correct.
Yes, I'm aware of the tracking functionality.
This error would be a major problem to find. So I would not recommend your fix above. A better way would be to restrict f() to function which return a const reference to the type returned by the current f().
I suppose I could also wrap the return type in a "wrapper type": template <class T> struct wrap_rvalue { wrap_rvalue(T const& x) : x(x) {} T x; }; template <class T> wrap_rvalue<T> rval(T const& x) { return wrap_rvalue<T>(x); } namespace boost { namespace serialization { template <class T> struct is_wrapper<wrap_rvalue<T> > : mpl::true_ {}; }} ar << rval( f() );
If that isn't convenient, and one had nothing else to do, he could explore the implications of implementing your proposed specialization so that it traps at compile time if the object might be tracked. This would mean that something like int f() would work while something like my_object f() would likely trap unless my_object was specifically marked as no_tracking. Thinking about this just a tiny bit makes this seem attractive to me. Of course that doesn't mean much until one sets out to actually do it.
That's actually pretty easy to do; something like: -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
I suppose I could also wrap the return type in a "wrapper type":
template <class T> struct wrap_rvalue { wrap_rvalue(T const& x) : x(x) {} T x; };
template <class T> wrap_rvalue<T> rval(T const& x) { return wrap_rvalue<T>(x); }
namespace boost { namespace serialization { template <class T> struct is_wrapper<wrap_rvalue<T> > : mpl::true_ {}; }}
ar << rval( f() );
That seems a worthy idea to me. The net effect would be to turn off tracking for this particular object. So, I would I would like to see this have a more widely understood name like dont_track(..) and added to the "case studies" section of the documentation. I think that the idea of temporarily turning off tracking (or other serialization traits) would have a usage beyond rvalues.
If that isn't convenient, and one had nothing else to do, he could explore the implications of implementing your proposed specialization so that it traps at compile time if the object might be tracked. This would mean that something like int f() would work while something like my_object f() would likely trap unless my_object was specifically marked as no_tracking. Thinking about this just a tiny bit makes this seem attractive to me. Of course that doesn't mean much until one sets out to actually do it.
That's actually pretty easy to do;
LOL - well for you maybe. Note one thing. Its not so much the code change, its all the other stuff that this generates: a) code change => b) documentation change/ehancement. This takes some effort to explain why serialzation is trapped or not trapped as a "side effect" of setting the tracking attribute. c) user support - Even if one takes the requisite effort to explain it, one will have to continually re-explain it as users have what they consider surprising behavior c) new tests - not a huge deal but still a minor pain - UNLESS something doesn't work with this or that obscure compiler. I'll consider it in more detail when I have the time. Robert Ramey P.S. I quick peek at the code below make me think that the line (serialization::tracking_level<T>::value == track_never), might better be replaced with: (serialization::tracking_level<T>::value <= track_never), RR
Index: interface_oarchive.hpp =================================================================== --- interface_oarchive.hpp (revision 3212) +++ interface_oarchive.hpp (working copy) @@ -28,6 +28,7 @@ class shared_ptr; namespace serialization { class extended_type_info; + template <class T> struct tracking_level; } // namespace serialization namespace archive { namespace detail { @@ -64,6 +65,14 @@ this->This()->save_override(t, 0); return * this->This(); } + + template<class T> + typename enable_if_c< + (serialization::tracking_level<T>::value == track_never), Archive & + >::type operator<<(T const& t){ + this->This()->save_override(t, 0); + return * this->This(); + }
// the & operator template<class T>
-- Dave Abrahams BoostPro Computing http://www.boostpro.com
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
on Sun Nov 09 2008, "Robert Ramey" <ramey-AT-rrsd.com> wrote:
LOL - well for you maybe.
Note one thing. Its not so much the code change, its all the other stuff that this generates:
a) code change =>
b) documentation change/ehancement. This takes some effort to explain why serialzation is trapped or not trapped as a "side effect" of setting the tracking attribute.
c) user support - Even if one takes the requisite effort to explain it, one will have to continually re-explain it as users have what they consider surprising behavior
c) new tests - not a huge deal but still a minor pain - UNLESS something doesn't work with this or that obscure compiler.
I'll consider it in more detail when I have the time.
Yeah, I'm familiar with that response. I've given it many times myself ;-)
Robert Ramey
P.S. I quick peek at the code below make me think that the line (serialization::tracking_level<T>::value == track_never), might better be replaced with: (serialization::tracking_level<T>::value <= track_never),
Easier for me, you say? I never would have gotten that. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
I'll consider it in more detail when I have the time.
Yeah, I'm familiar with that response. I've given it many times myself ;-)
You might want to package this as a suggested patch and open a track ticket on it. I do address these tickets as I have time available. Robert Ramey
Robert Ramey
participants (3)
-
David Abrahams -
Robert Ramey -
Thorsten Ottosen