[boost] Re: serialization: review

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-04-19 20:25:47

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