Re: [boost] Re: serialization: review

Thorsten Ottosen wrote:
Robert Ramey wrote:(> |)
| I don't understand the need for this. Given that mammal is an abstract | base class with no state variables of its own, it needs no serialization | at all.
Ok, but could I have implemented it by having just one function in the abstract base class which seriaized by calling virtual functions, eg, name() and age()?
I believe you could. In fact I believe that in some cases not yet encountered ( runtime plug-ins ) it will be necessary. This is still an unresolved area
| Just declare it as abstract using BOOST_IS_ABSTRACT(mammal). Only the | serialization of the derived classes need be specified.
Can BOOST_IS_ABSTRACT be avoided? Maybe some type-traits can figure this out?
Is_abstract is in the new type traits. It fails on lots of compilers. Also it was in a state of flux at the time of the release of 1.31. I expect that a) is_abstract get fixed up best it can b) and have an implementation that is benign for compilers that don't support it c) and that we'll continue to specialize it for serialization code that needs to be compiled on less-conforming compilers. (this is all BOOST_IS_ABSTRACT does.) In
struct whale : public mammal { friend class boost::serialization::access; whale(); whale( const std::string& name, unsigned age ); virtual const std::string& name() const; virtual unsigned age() const; private: template<class Archive> void serialize(Archive &ar, const unsigned int version){ ar & boost::serialization::base_object<mammal>(*this); ar & name_ & age_ ; } std::string name_; unsigned age_; };
can the line "ar & boost::serialization::base_object<mammal>( *this )" be put in a base-class like this
mammal::serialize( Archive& ar, unsigned version ) { ar & boost::serialization::base_object<mammal>(*this); do_serialize( ar, version ); // virtual function that does the rest }
to remove some redundancy?
The intention of base_object is really to cast to the base class (and register the base/derived relationship). So it its out of place here. The real issue is do_serialize. Is that supposed to be a virtual function? With a templated argument? Maybe you want to consider this question a little bit more.
| I would love to hear more about this. I would hope that it would easy | to be totally successful.
it is now given you showed me how :-) In effect, once the container supports serialization, a class can be supported in four lines. If you have not got an example of class hierarchies, then maybe it should be added.
The tutorial contains an example essentially identical to your mammals, whales and apes example. Under case studies there's a discussion of the implementation of serialization for shared_ptr which is almost exactly analogous to your vector_ptr case. Also, there some discussion in the documentation regarding serializaition of templates.
| > I would like to see the focus shift from vector<T*> towards vector< | > shared_ptr<T> > and ptr_vector<T>. using T* array[sz] or vector<T*> | > is inherently unsafe if the pointers are heap-allocated. | | I believe that enforcing this is outside my role as a library creator.
ok, but I think you at least should point out very clearly the problems in it. Some non-experienced programmer might do what he sees in documentation; therefore examples should encourage good style.
Well, here is one programmer that needs to be better sold on this. Honestly, I don't think the serialization tutorial is the place to do it.
| 3. const members uses a const-cast to re-init them. AFAICT, this is | undefined behavior (cmp 7.1.5.1). I guess the need for serialization | is just another reason for not having const members.
Hmmm - what does re-init mean in this context?
| > this has come up from time to time. I've tried to promote the view | > that const members shouldn't be serialized but I couldn't convince | > everyone. | > I consider it outside my role as a library developer to try to | > enforce this.
my point was that the examples given give undefined behavior so that solution cannot be used.
Which example gives undefined behavior?
no, but maybe with this
| stl::archive_input_seq<Archive, std::vector>, | stl::reserve_imp<std::vector>
by arranging the order of the template parameters.
| Isn't a little bit of redundancy OK giving the alternative of having | to keep the details of deducibility in one's brain?
but you already know the types that will be deduced from the outer function.
OK, could we agree to disagree on this and forget about it?
| > 18. implementation of save_collection(). you cannot use unsigned int | > count portably. | | I don't see this. | | > Instead you should use the new collection traits (reviewed after | > this review) as follows: | | > typename boost::size_type_of< Collection >::type count = size( s ); | | > then the code will also work for iterator views and arrays. Similar | > comments apply to load_collection_impl.hpp | | I wanted to make the library buildable under 1.31 .
ok, then do
typename Collection::size_type count = s.size();
but don't assume Collection::size_type will be unsigned int.
I'm not assuming that. I'm assuming that it will be convertible to an unsigned int and that an unsigned int will be large enough to hold any number of elements in an collection. Of course THAT might be rash assumption. BTW where is the namespace Collection defined in 1.31 ? Robert Ramey

"Robert Ramey" <ramey@rrsd.com> wrote in message news:20040420155911.280A431574@acme.west.net... [snip] | > can the line "ar & boost::serialization::base_object<mammal>( *this )" | > be put in a base-class like this | | > mammal::serialize( Archive& ar, unsigned version ) { | > ar & boost::serialization::base_object<mammal>(*this); | > do_serialize( ar, version ); | > // virtual function that does the rest | > } | | > to remove some redundancy? | | The intention of base_object is really to cast to the base class (and | register the base/derived relationship). So it its out of place here. The | real issue is do_serialize. Is that supposed to be a virtual function? With | a templated argument? Maybe you want to consider this question a little bit | more. then think of the do_serialize as an overloaded virual function. My point is that some might not want to have serialization stuff in a header. | > | > I would like to see the focus shift from vector<T*> towards vector< | > | > shared_ptr<T> > and ptr_vector<T>. using T* array[sz] or vector<T*> | > | > is inherently unsafe if the pointers are heap-allocated. | > | | > | I believe that enforcing this is outside my role as a library creator. | | > ok, but I think you at least should point out very clearly the problems in | > it. Some non-experienced programmer might do what he sees in | > documentation; therefore examples should encourage good style. | | Well, here is one programmer that needs to be better sold on this. | Honestly, I don't think the serialization tutorial is the place to do it. You're right in the sense that nowhere is the place to do it :-) Anyway, I forgot to ask, what happens if I seralize a std::list<T*> where the objects are not heap-allocated, but merely pointers to other T's.? How do one control whether to heap-allocate or not? | > | 3. const members uses a const-cast to re-init them. AFAICT, this is | > | undefined behavior (cmp 7.1.5.1). I guess the need for serialization | > | is just another reason for not having const members. | | Hmmm - what does re-init mean in this context? "re-init" means assigning to a const member after it has been initialized. | > | > this has come up from time to time. I've tried to promote the view | > | > that const members shouldn't be serialized but I couldn't convince | > | > everyone. | > | > I consider it outside my role as a library developer to try to | > | > enforce this. | | > my point was that the examples given give undefined behavior so that | > solution cannot be used. | | Which example gives undefined behavior? this example below, unless I misunderstand 7.1.5.1 of the standard. const Members Saving const members to an archive requires no special considerations. Loading const members can be addressed by using a const_cast: ar & const_cast<T &>(t); | > | > 18. implementation of save_collection(). you cannot use unsigned int | > | > count portably. | > | | > | I don't see this. | > | | > | > Instead you should use the new collection traits (reviewed after | > | > this review) as follows: | > | | > | > typename boost::size_type_of< Collection >::type count = size( s ); | > | | > | > then the code will also work for iterator views and arrays. Similar | > | > comments apply to load_collection_impl.hpp | > | | > | I wanted to make the library buildable under 1.31 . | | > ok, then do | | > typename Collection::size_type count = s.size(); | | > but don't assume Collection::size_type will be unsigned int. | | I'm not assuming that. I'm assuming that it will be convertible to an | unsigned int and that an unsigned int will be large enough to hold any | number of elements in an collection. Of course THAT might be rash | assumption. bingo. | BTW where is the namespace Collection defined in 1.31 ? I assumed the template parameter would be called that. br Thorsten
participants (2)
-
Robert Ramey
-
Thorsten Ottosen