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
struct mammal : boost::noncopyable { private: friend class boost::serialization::access; virtual void do_serialize( boost::archive::text_oarchive& ar, unsigned version ) = 0; virtual void do_serialize( boost::archive::text_iarchive& ar, unsigned version ) = 0; template< class Archive > void serialize( Archive & ar, unsigned version ) { do_serialize( ar, version ); } public: virtual const std::string& name() const = 0; virtual unsigned age() const = 0; virtual ~mammal() {} };
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. Just declare it as abstract using BOOST_IS_ABSTRACT(mammal). Only the serialization of the derived classes need be specified.
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.
a.. What is your evaluation of the documentation?
I miss more specific type requirements. For example, is it necessary to have a default constructor? Is it necessary to be stack-allocable? Must the destructor be public?
Good point.
I did try to extend the library to use my new smart containers; I was almost successful.
I would love to hear more about this. I would hope that it would easy to be totally successful.
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.
I tried to extend the library to support my smart containers, but I couldn't figure out what I did wrong. I think I might have saved them correctly, but I cannot figure out to load them. However, I do think that is a problem that can be solved together with Robert. (Robert, you can find the ptr_vector in the sandbox. If you could cast a glance on the code and tell me how the body of load_collection should look like, it would be great.)
I'll look at it. Thorsten
Some general nitpicks/comments (with these comments my test works on vc71, como4.3.3, g++3.3.1:
1. have you planned to use concept checks
I'm intrigued with concept checks for something like this but I havn't had the time to spend on it.
2. documentation: can the width of the html be made adaptive to the window width? The right window must be scrolled to see it's contents which is annoying.
This drives me crazy also. Unfortunately, I haven't been able to figure out what causes this. I believe it's a Microsoft IE issue - but who knows. I would be grateful to anyone can explain to me how to fix this. 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.
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.
5. I think the exception code should be returned by a function: exception_code code() const; this is how eg. the regex_error exception in tr1 will do it.
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.
7. archive/detail/basic_oarchive.hpp includes itself
Whoops.
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.
9. serialization/serialization.hpp: why all the static casts?
template<class Archive, class T> inline void serialize( Archive & ar, T & t, const BOOST_PFTO unsigned int file_version ){ access::serialize(ar, t, static_cast<const unsigned int>(file_version)); }
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.
10. nvp.hpp: line 50: ar & t.value(); There is on t-parameter.
OK - interesting the default implementation was never instantiated.
11. /archive/detail/oserializer.hpp", line 244: NULL reference is not allowed return * static_cast<const basic_pointer_oserializer *>(NULL); ^ This not yet legal C++ (but the committee is changing that) use a static member and return that instead.
Also, why all the static_casts?
In this case the reason is that when handling issues regarding preamble, object-id, class id etc. the pointer to the object is handled as void * . This permits the archive library to be pre-compiled and not have to be recompiled for every instance. When such a void pointer is passed back to a specific archive-type combination it has to be case back to the orginal type to permit the calling of type serialization functions. This is really a result reconciliation of conflicting goals. a) serialization of pointers abstract and otherwise b) serialization of pointers in a non-intrusive manner c) limit pointless instantiation. d) not require that user classes import headers related solely to implementation. For pointers this has generated a design which takes the typed pointer converts it to void pointer, handles preamble and book keeping, converts it back to typed pointer and (de)serializes the data. The requirement of non-intrusiveness means we can't just pass around a base class pointer - hence the need for casting.
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. archive::save(* common_oarchive<Archive>::This(), t);
remark: why is this->This()->XX() necessary? this->xx() should do it.
Hmm - I don't see this. save(Archive & ar, T t) is implemented in a more derived class. Common_oarchive<Archive> rededirects it downward to that class. I would expect this->xx() to give and undefined error. I could use archive::save(* static_cast<Archive *>(this)t); though I'm not sure you would find that anymore appealing.
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.
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.
15: /archive/detail/archive_pointer_iserializer.hpp", line 42: error #284: (+detail/iserializer.hpp", line 240:) NULL reference is not allowed return *static_cast<const basic_iserializer *>(NULL); ^ Solution: use a static member again.
16: two-phase lookup problems in archive/basic_text_iarchive.hpp", Solution: prefix by this->.
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 ); } 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< Archive, std::vector stl::archive_input_seq<Archive, std::vector>, stl::reserve_imp<std::vector> >(ar, t); } Isn't a little bit of redundancy OK giving the alternative of having to keep the details of deducibility in one's brain?
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 .
19. missing #include <boost/serialization/nvp.hpp> in serialization.hpp
Whoops - thanks. Robert Ramey