|
|
Boost : |
From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-04-19 07:21:04
Hi Robert,
Here is my review of your library.
a.. What is your evaluation of the design?
Seems fine. I have a few issues though.
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() {}
};
Some way of supporting only one do_serialize() to preserve the order would
be nice.
a.. What is your evaluation of the implementation?
There are some minor compiler problems and suggestions listed below. I can't
figure out
exactly how the library save/loads pointers when these cannot be allocated
on the stack.
a.. What is your evaluation of the documentation?
I already gave some feedback on the introduction. In general it seems ok. I
like the comparison
with other lbraries. 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?
a.. What is your evaluation of the potential usefulness of the library?
It's certainly useful.
a.. Did you try to use the library? With what compiler? Did you have any
problems?
yes, see below. I can compile my code with vc71, gcc3.3 comeau 4.3.3.
a.. How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
5 hours. It's neither a glance nor an in-depth study. I did try to extend
the library to use my new
smart containers; I was almost sucessful.
a.. Are you knowledgeable about the problem domain?
hm...no.
a.. Do you think the library should be accepted as a Boost library? Be
sure to say this explicitly so that your other comments don't obscure your
overall opinion
yes.
But I think the pointer part of the library need to be given extra thought;
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
tried to extend the library to
support my smart containers, but I coulnd'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.)
freestanding libraries: Obvious candidates are
(a) BOOST_STRONG_TYPEDEF (could maybe be part of utility)
(b) dataflow_iterators
best regards
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
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.
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.
4. AFAICR, the use of placement new require proper alignment, so the object
must eg. be heap-allocated. Is that guaranteed?
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"
7. archive/detail/basic_oarchive.hpp includes itself
8. is_abstract.hpp: there is something wierd about the is_abstact struct. I
uncommented the first part to be able to compile.
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
10. nvp.hpp: line 50: ar & t.value(); There is on t-parameter.
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?
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->. remark: why is
this->This()->XX() necessary? this->xx() should do it.
13: archive/text_oarchive.hpp, similar lookup problems. this->newtoken(),
this->delimiter = this->none;
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.
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.
18. implementation of save_collection(). you cannot use unsigned int count
portably. 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
19. missing #include <boost/serialization/nvp.hpp> in serialization.hpp
begin 666 smart_container.cpp
M(VEN8VQU9&4@/&9S=')E86T^#0HC:6YC;'5D92 \<W1R:6YG/@T*(VEN8VQU
M9&4@/&EO<W1R96%M/@T*(VEN8VQU9&4@/&)O;W-T+V%R8VAI=F4O=&5X=%]O
M87)C:&EV92YH<' ^#0HC:6YC;'5D92 \8F]O<W0O87)C:&EV92]T97AT7VEA
M<F-H:79E+FAP<#X-"B-I;F-L=61E(#QB;V]S="]S97)I86QI>F%T:6]N+W-T
M<FEN9RYH<' ^#0HC:6YC;'5D92 \8F]O<W0O<V5R:6%L:7IA=&EO;B]V97)S
M:6]N+FAP<#X-"B-I;F-L=61E(#QB;V]S="]S97)I86QI>F%T:6]N+W-P;&ET
M7VUE;6)E<BYH<' ^#0HC:6YC;'5D92 \8F]O<W0O<V5R:6%L:7IA=&EO;B]V
M96-T;W(N:'!P/@T*(VEN8VQU9&4@/&)O;W-T+W-E<FEA;&EZ871I;VXO97AP
M;W)T+FAP<#X-"B-I;F-L=61E(#QB;V]S="]P=')?8V]N=&%I;F5R+W!T<E]V
M96-T;W(N:'!P/@T*(VEN8VQU9&4@/&)O;W-T+V%S<VEG;B]S=&PN:'!P/@T*
M(VEN8VQU9&4@/&)O;W-T+VYO;F-O<'EA8FQE+FAP<#X-"@T*+R\O+R\O+R\O
M+R\O+R\O+R\O+R\O+R\O+R\O+PT*+R\@<'1R('9E8W1O<B!S97)I86QI>F%T
M:6]N#0H-"B-I;F-L=61E(#QB;V]S="]S97)I86QI>F%T:6]N+V-O;&QE8W1I
M;VYS7W-A=F5?:6UP+FAP<#X-"B-I;F-L=61E(#QB;V]S="]S97)I86QI>F%T
M:6]N+V-O;&QE8W1I;VYS7VQO861?:6UP+FAP<#X-"B-I;F-L=61E(#QB;V]S
M="]S97)I86QI>F%T:6]N+W-P;&ET7V9R964N:'!P/@T*#0IT96UP;&%T93QC
M;&%S<R!!<F-H:79E+"!C;&%S<R!5+"!C;&%S<R!!;&QO8V%T;W(^#0II;FQI
M;F4@=F]I9"!S879E* T*(" @($%R8VAI=F4@)B!A<BP-"B @("!C;VYS="!B
M;V]S=#HZ<'1R7W9E8W1O<CQ5+"!!;&QO8V%T;W(^("9T+ T*(" @('5N<VEG
M;F5D(&EN="!F:6QE7W9E<G-I;VX-"BD-"GL-"B @("!B;V]S=#HZ<V5R:6%L
M:7IA=&EO;CHZ<W1L.CIS879E7V-O;&QE8W1I;VXH(&%R+"!T("D[#0I]#0H-
M"G1E;7!L871E/&-L87-S($%R8VAI=F4L(&-L87-S(%4L(&-L87-S($%L;&]C
M871O<CX-"FEN;&EN92!V;VED(&QO860H#0H@(" @07)C:&EV92 F(&%R+ T*
M(" @(&)O;W-T.CIP=')?=F5C=&]R/%4L($%L;&]C871O<CX@)G0L#0H@(" @
M=6YS:6=N960@:6YT(&9I;&5?=F5R<VEO;@T**0T*>PT*+RH@,2X_at_8V]M<&EL
M97,L(&)U="!C<F%S:&5S#0H@(" @(" @('-T;#HZ;&]A9%]C;VQL96-T:6]N
M/ T*(" @(" @("!!<F-H:79E+ T*(" @(" @("!B;V]S=#HZ<'1R7W9E8W1O
M<CQ5+"!!;&QO8V%T;W(^+ T*(" @(" @("!S=&PZ.F%R8VAI=F5?:6YP=71?
M<V5Q/$%R8VAI=F4L(&)O;W-T.CIP=')?=F5C=&]R/%4L($%L;&]C871O<CX@
M/BP-"B @(" @(" @<W1L.CIR97-E<G9E7VEM<#QB;V]S=#HZ<'1R7W9E8W1O
M<CQ5+"!!;&QO8V%T;W(^(#X-"B @(" ^*&%R+"!T*3L-"B @(" J+PT*?0T*
M#0HO+R!S<&QI="!N;VXM:6YT<G5S:79E('-E<FEA;&EZ871I;VX_at_9G5N8W1I
M;VX@;65M8F5R(&EN=&\@<V5P87)A=&4-"B\O(&YO;B!I;G1R=7-I=F4@<V%V
M92]L;V%D(&UE;6)E<B!F=6YC=&EO;G,-"G1E;7!L871E/&-L87-S($%R8VAI
M=F4L(&-L87-S(%4L(&-L87-S($%L;&]C871O<CX-"FEN;&EN92!V;VED('-E
M<FEA;&EZ92_at_-"B @("!!<F-H:79E("8_at_87(L#0H@(" @8F]O<W0Z.G!T<E]V
M96-T;W(\52P_at_06QL;V-A=&]R/B F('0L#0H@(" @8V]N<W0@=6YS:6=N960@
M:6YT(&9I;&5?=F5R<VEO;@T**7L-"B @("!S<&QI=%]F<F5E*&%R+"!T+"!F
M:6QE7W9E<G-I;VXI.PT*?0T*#0H-"B\O+R\O+R\O+R\O+R\O+R\O+PT*+R\@
M8VQA<W,@:&EE<F%C:'D-"@T*<W1R=6-T(&UA;6UA;" Z(&)O;W-T.CIN;VYC
M;W!Y86)L90T*>PT*<')I=F%T93H-"B @(" -"B @("!F<FEE;F0_at_8VQA<W,@
M8F]O<W0Z.G-E<FEA;&EZ871I;VXZ.F%C8V5S<SL-"B @("!V:7)T=6%L('9O
M:60_at_9&]?<V5R:6%L:7IE*"!B;V]S=#HZ87)C:&EV93HZ=&5X=%]O87)C:&EV
M928_at_87(L('5N<VEG;F5D('9E<G-I;VX@*2 ](# [#0H@(" @=FER='5A;"!V
M;VED(&1O7W-E<FEA;&EZ92@@8F]O<W0Z.F%R8VAI=F4Z.G1E>'1?:6%R8VAI
M=F4F(&%R+"!U;G-I9VYE9"!V97)S:6]N("D@/2 P.PT*(" @( T*(" @('1E
M;7!L871E/"!C;&%S<R!!<F-H:79E(#X-"B @("!V;VED('-E<FEA;&EZ92@@
M07)C:&EV92 F(&%R+"!U;G-I9VYE9"!V97)S:6]N("D-"B @("![#0H@(" @
M(" @(&1O7W-E<FEA;&EZ92@@87(L('9E<G-I;VX@*3L-"B @("!]#0H@(" @
M#0IP=6)L:6,Z#0H-"B @("!V:7)T=6%L(&-O;G-T('-T9#HZ<W1R:6YG)B!N
M86UE*"D_at_8V]N<W0@/2 P.PT*(" @('9I<G1U86P@=6YS:6=N960@(" @(" @
M(" @(&%G92_at_I(&-O;G-T(" ](# [#0H@(" @(" @(" @#0H@(" @=FER='5A
M;"!^;6%M;6%L*"D@>WT-"GT[#0H-"G-T9#HZ;W-T<F5A;28@;W!E<F%T;W(\
M/"@@<W1D.CIO<W1R96%M)B!O<RP_at_8V]N<W0@;6%M;6%L)B!M("D-"GL-"B @
M("!R971U<FX@;W,@/#P@(FYA;64@/2 B(#P\(&TN;F%M92_at_I(#P\("(@86=E
M(#T@(B \/"!M+F%G92_at_I.R -"GT-"@T*<W1R=6-T(&%P92 Z('!U8FQI8R!M
M86UM86P-"GL-"B @("!A<&4H*3L-"B @("!A<&4H(&-O;G-T('-T9#HZ<W1R
M:6YG)B!N86UE+"!U;G-I9VYE9"!A9V4@*3L-"B @("!V:7)T=6%L(&-O;G-T
M('-T9#HZ<W1R:6YG)B!N86UE*"D_at_8V]N<W0[#0H@(" @=FER='5A;"!U;G-I
M9VYE9" @(" @(" @(" @86=E*"D_at_8V]N<W0[#0IP<FEV871E._at_T*(" @('9I
M<G1U86P@=F]I9"!D;U]S97)I86QI>F4H(&)O;W-T.CIA<F-H:79E.CIT97AT
M7V]A<F-H:79E)B!A<BP@=6YS:6=N960@=F5R<VEO;B I.PT*(" @('9I<G1U
M86P@=F]I9"!D;U]S97)I86QI>F4H(&)O;W-T.CIA<F-H:79E.CIT97AT7VEA
M<F-H:79E)B!A<BP@=6YS:6=N960@=F5R<VEO;B I.PT*(" @( T*(" @('-T
M9#HZ<W1R:6YG(" @;F%M95\[#0H@(" @=6YS:6=N960@(" @("!A9V5?.PT*
M?3L-"@T*<W1R=6-T('=H86QE(#H@<'5B;&EC(&UA;6UA; T*>PT*(" @('=H
M86QE*"D[#0H@(" @=VAA;&4H(&-O;G-T('-T9#HZ<W1R:6YG)B!N86UE+"!U
M;G-I9VYE9"!A9V4@*3L-"B @("!V:7)T=6%L(&-O;G-T('-T9#HZ<W1R:6YG
M)B!N86UE*"D_at_8V]N<W0[#0H@(" @=FER='5A;"!U;G-I9VYE9" @(" @(" @
M(" @86=E*"D_at_8V]N<W0[#0IP<FEV871E._at_T*(" @('9I<G1U86P@=F]I9"!D
M;U]S97)I86QI>F4H(&)O;W-T.CIA<F-H:79E.CIT97AT7V]A<F-H:79E)B!A
M<BP@=6YS:6=N960@=F5R<VEO;B I.PT*(" @('9I<G1U86P@=F]I9"!D;U]S
M97)I86QI>F4H(&)O;W-T.CIA<F-H:79E.CIT97AT7VEA<F-H:79E)B!A<BP@
M=6YS:6=N960@=F5R<VEO;B I.PT*(" @( T*(" @('-T9#HZ<W1R:6YG(" @
M;F%M95\[#0H@(" @=6YS:6=N960@(" @("!A9V5?.PT*?3L-"@T*0D]/4U1?
M0TQ!4U-?15A03U)47T=5240H(&%P92P@(F%P92(@*0T*0D]/4U1?0TQ!4U-?
M15A03U)47T=5240H('=H86QE+" B=VAA;&4B("D-"@T*+R\O+R\O+R\O+R\O
M+R\O+R\-"B\O(&]R9&5R:6YG<PT*#0IS=')U8W0@:6Y?;F%M95]O<F1E<@T*
M>PT*(" @(&)O;VP@;W!E<F%T;W(H*2@@8V]N<W0@;6%M;6%L)B!L+"!C;VYS
M="!M86UM86PF('(@*0T*(" @('L-"B @(" @(" @<F5T=7)N(&PN;F%M92_at_I
M(#P@<BYN86UE*"D[#0H@(" @?0T*?3L-"@T*+R\O+R\O+R\O+R\O+R\O+R\-
M"B\O('1E<W0-"@T*:6YT(&UA:6XH*0T*>PT*(" @('1R>0T*(" @('L-"@T*
M(" @(" @("!U<VEN9R!N86UE<W!A8V4_at_8F]O<W0Z.F%S<VEG;FUE;G0[#0H@
M(" @(" @( T*(" @(" @("!B;V]S=#HZ<'1R7W9E8W1O<CQM86UM86P^('9E
M8SL-"B @(" @(" @+R\@87-S:6=N('!O<W-I8FEL:71Y.B!O;F4@;75S="!B
M92!A8FQE('1O('-P96-I9GD@=&AE('1Y<&4@=&\@8V]N<W1R=6-T#0H@(" @
M(" @("\O(&5G#0H@(" @(" @("\O('!T<E]A<W-I9VX\87!E/B@@=F5C("DH
M(E_at_B+"!Y("D[(&%S<VEG;CQA<&4^*"!V96,@*2 ](&YE=R!!<&4H(")8(BP@
M>2 I.R -"B @(" @(" @+R\@<'1R7V%S<VEG;CQW:&%L93XH('9E8R I*")8
M(BP@>2 I.R -"B @(" @(" @=F5C("L](&YE=R!A<&4H(")&<F%N:R(L(#$R
M("DL(&YE=R!A<&4H(")"971T>2(L(#D@*2P@;F5W('=H86QE*" B2F]N87,B
M+" S," I+"!N97<@=VAA;&4H(")'97)D82(L(#(T("D[#0H@(" @(" @("\O
M=F5C+G-O<G0H(&EN7VYA;65?;W)D97(H*2 I.PT*(" @(" @(" -"B @(" @
M(" @<W1D.CIO9G-T<F5A;2!O9G,H(")M86UM86QS+F1A="(@*3L-"B @(" @
M(" @8F]O<W0Z.F%R8VAI=F4Z.G1E>'1?;V%R8VAI=F4@;W5T7W1E>'0H(&]F
M<R I.PT*(" @(" @("!O=71?=&5X=" \/"!V96,[#0H@(" @(" @(&]F<RYC
M;&]S92_at_[hidden]*(" @(" @("!S=&0Z.FEF<W1R96%M(&EF<R@@(FUA;6UA;',N
M9&%T(B I.PT*(" @(" @("!B;V]S=#HZ87)C:&EV93HZ=&5X=%]I87)C:&EV
M92!I;E]T97AT*"!I9G,@*3L-"B @(" @(" @8F]O<W0Z.G!T<E]V96-T;W(\
M;6%M;6%L/B!C;W!Y.PT*(" @(" @("!I;E]T97AT(#X^(&-O<'D[#0H@(" @
M(" @( T*(" @(" @("!F;W(H('-I>F5?="!I(#T@,#L@:2 \('9E8RYS:7IE
M*"D[("LK:2 I#0H@(" @(" @(" @("!S=&0Z.F-O=70@/#P@=F5C6VE=(#P\
M(&-O<'E;:5T@/#P@)UQN)SL-"B @("!]#0H@(" @8V%T8V_at_H('-T9#HZ97AC
M97!T:6]N)B!E("D-"B @("![#0H@(" @(" @('-T9#HZ8V]U=" \/"!E+G=H
M870H*3L@#0H@(" @?0T*?0T*#0HO+R\O+R\O+R\O+R\O+R\O+R\O+R\O#0HO
M+R!I;7!L96UE;G1A=&EO;B!A<&4-"@T*87!E.CIA<&4H*0T*>PT*?0T*#0IA
M<&4Z.F%P92@@8V]N<W0@<W1D.CIS=')I;F<F(&YA;64L('5N<VEG;F5D(&%G
M92 I(#H@;F%M95\H(&YA;64@*2P_at_86=E7R@@86=E("D-"GL-"GT-"B @(" -
M"F-O;G-T('-T9#HZ<W1R:6YG)B!A<&4Z.FYA;64H*2!C;VYS= T*>PT*(" @
M(')E='5R;B!N86UE7SL-"GT-"B @(" -"G5N<VEG;F5D(&%P93HZ86=E*"D@
M8V]N<W0-"GL-"B @("!R971U<FX_at_86=E7SL-"GT-"B @(" -"G9O:60_at_87!E
M.CID;U]S97)I86QI>F4H(&)O;W-T.CIA<F-H:79E.CIT97AT7V]A<F-H:79E
M)B!A<BP@=6YS:6=N960@=F5R<VEO;B I#0I[#0H@(" @87(@/#P@;F%M95\@
M/#P_at_86=E7SL-"GT-"@T*=F]I9"!A<&4Z.F1O7W-E<FEA;&EZ92@@8F]O<W0Z
M.F%R8VAI=F4Z.G1E>'1?:6%R8VAI=F4F(&%R+"!U;G-I9VYE9"!V97)S:6]N
M("D-"GL-"B @("!A<B ^/B!N86UE7R ^/B!A9V5?.PT*?0T*#0HO+R\O+R\O
M+R\O+R\O+R\O+R\O+R\O+R\O#0HO+R!I;7!L96UE;G1A=&EO;B!W:&%L90T*
M#0IW:&%L93HZ=VAA;&4H*0T*>PT*?0T*#0IW:&%L93HZ=VAA;&4H(&-O;G-T
M('-T9#HZ<W1R:6YG)B!N86UE+"!U;G-I9VYE9"!A9V4@*2 Z(&YA;65?*"!N
M86UE("DL(&%G95\H(&%G92 I#0I[#0I]#0H@(" @#0IC;VYS="!S=&0Z.G-T
M<FEN9R8@=VAA;&4Z.FYA;64H*2!C;VYS= T*>PT*(" @(')E='5R;B!N86UE
M7SL-"GT-"B @(" -"G5N<VEG;F5D('=H86QE.CIA9V4H*2!C;VYS= T*>PT*
M(" @(')E='5R;B!A9V5?.PT*?0T*(" @( T*=F]I9"!W:&%L93HZ9&]?<V5R
M:6%L:7IE*"!B;V]S=#HZ87)C:&EV93HZ=&5X=%]O87)C:&EV928_at_87(L('5N
M<VEG;F5D('9E<G-I;VX@*0T*>PT*(" @(&%R(#P\(&%G95\@/#P@;F%M95\[
M#0I]#0H-"G9O:60@=VAA;&4Z.F1O7W-E<FEA;&EZ92@@8F]O<W0Z.F%R8VAI
M=F4Z.G1E>'1?:6%R8VAI=F4F(&%R+"!U;G-I9VYE9"!V97)S:6]N("D-"GL-
A"B @("!A<B ^/B!A9V5?(#X^(&YA;65?.R -"GT-"@T*
`
end
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk