Boost logo

Boost :

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
| > cannot use & anymore. Consider this

| I don't understand the need for this. Given that mammal is an abstract
| class with no state variables of its own, it needs no serialization at

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?

struct whale : public mammal
    friend class boost::serialization::access;
    whale( const std::string& name, unsigned age );
    virtual const std::string& name() const;
    virtual unsigned age() const;
    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
| > 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
| 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 I guess the need for serialization is
| 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
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
| > 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
| 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->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
| >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
| > 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<
[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
| 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
| > 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.



Boost list run by bdawes at, gregod at, cpdaniel at, john at