Hi Robert, I have some more stupid questions. | Thorsten Ottosen wrote: | | > Why isn't there a base class for all archieves? I think that would be | useful | | > with class hierachies...for example, I need to implement two function and | > cannot use & anymore. Consider this | 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()? | 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? 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? | > I can't figure out exactly how the library save/loads pointers when these | > cannot be allocated on the stack. | | Please expand some more on this. I think you explain more about it later. Basically, I couldn't figure out the scheme with void*s and cast etc. | 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. | > 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-experinced programmer might do what he sees in documentation; therefore examples should encourage good style. | 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. | | > 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.Unless there is something I overlooked. | > 4. AFAICR, the use of placement new require proper alignment, so the | > object must eg. be heap-allocated. Is that guaranteed? | | I believe this has been addressed. Objects allocated on the stack use | aligned storage to handle this. yes, neat. | > 6. remove trailing comma in archive_exception.hpp: "unregistered_cast // | > base - derived relationship not registered with" | | OK - though I don't think its an error. Comeau call's it non-standard. | > 8. is_abstract.hpp: there is something wierd about the is_abstact struct. | > uncommented the first part to be able to compile. | | I don't see this. // so default to false until the above is really fixed namespace boost { \ template<class T> struct is_abstract { typedef mpl::false_ type; BOOST_STATIC_CONSTANT(bool, value = is_abstract::type::value); }; } // namespace boost what is the '\' doing there? I just uncommented the whole thing. | > 9. serialization/serialization.hpp: why all the static casts? | > AFAICT, they should be removed. Comeau has the follwing warning: type | > qualifier is meaningless on cast type | | They wouldn't be there except they provide a work around for compilers which | do not support Partial Template Function Specialization. On conforming | compilers, the BOOST_PFTO resolves to nothing so the cast wouldn't be | necessary. So commeau is correct. I don't know how much effort I want to | invest to suppress benign warnings for specific compilers. ok, we should consider either to remove warnings when using bjam or come up with something. | > 12. two-phase lookup problems: | > /boost/archive/basic_text_oarchive.hpp", line 65: error #20: | > identifier "This" is undefined | > archive::save(* This(), t); | ^ | > Solution: prefix base class functions with this->. | | I'm not sure I understand this but I don't see it doing any harm. If this | is a problem, the following would make more sense to me. when you access something in a dependent base class, Base, one must prefix function with either this-> or Base::. The first preserves virtuality, the second don't. Similarly, typedefs need to be prefixed by Base::. | archive::save(* common_oarchive<Archive>::This(), t); the problem is that This() is defined in a dependent base class. | > 13: archive/text_oarchive.hpp, similar lookup problems. this->newtoken(), | > this->delimiter = this->none; | | as before - I don't see a problem but changing it doesn't hurt anything. it makes all the difference on comforming compilers. | > 14: /archive/detail/basic_iarchive.hpp", line 46: error #20: | > identifier "basic_iarchive_impl" is undefined | > boost::scoped_ptr<basic_iarchive_impl> pimpl; | ^ | >solution: remember to forward declare basic_iarchieve_impl. Do the same for | >basic_oarchieve_impl. | | OK - I had thought that using friend class made that unnecessary. I'm not sure who's right here, but at least it will make it work with one more compiler. | > 17. when you save_collection_impl.hpp (eg in vector.hpp) there is no need | > to specify template arguments again since they will be deduced. | | I presume you mean that: | | template<class Archive, class U, class Allocator> | inline void save( | Archive & ar, | const std::vector<U, Allocator> &t, | const unsigned int file_version | ){ | stl::save_collection<Archive, std::vector<U, Allocator> >( | ar, t | ); | } | | Can be replaced with: | | template<class Archive, class U, class Allocator> | inline void save( | Archive & ar, | const std::vector<U, Allocator> &t, | const unsigned int file_version | ){ | stl::save_collection<Archive, std::vector>( | ar, t | ); | } yes. | But what about: | | template<class Archive, class U, class Allocator> | inline void load( | Archive & ar, | std::vector<U, Allocator> &t, | const unsigned int file_version | ){ | stl::load_collection< | Archive, | std::vector<U, Allocator>, | stl::archive_input_seq<Archive, std::vector<U, Allocator> >, | stl::reserve_imp<std::vector<U, Allocator> > | >(ar, t); | } | | This can't be replaced with: | | template<class Archive, class U, class Allocator> | inline void load( | Archive & ar, | std::vector<U, Allocator> &t, | const unsigned int file_version | ){ | stl::load_collection< [snip, correction to no list here] | >(ar, t); | } 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. | > 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. br Thorsten