Message: 13 Fredrik Blomqvist
* What is your evaluation of the implementation? As I mentioned in my previous review I would like to see it handle most of the STL and core boost components "out of the box".
I believe all STL is handled. Serialization of other boost components will have to be the responsibility of their authors or others.
In particular I think the smart_ptrs (including auto_ptr) should have core-support from the serializer, even special cased if necessary (similar to boost::bind etc).
I believe the current library is capable of implementing serialization for all boost components. Much effort was expended to that the library would not have to have special code for any serialization or archive type. In fact, my interest is just the opposite. I would like to see a significant part of the library moved outside. Anyone who wants to volunteer to take responsibility for any of the component please let us know. The library would be improved by making it smaller.
The code to serialize std::auto_ptr could easily be copied to the official directory for example.
There has been some discussion of where the serialization code for other boost libraries should reside. I don't have strong feelings about this other than I would prefer that it reside outside the serialization library itself. The only exception I would make is for the STL serialization which has no where else in boost to reside and it has a "special" status as and official C++ library.
Regarding shared_ & weak_ptr. If the implementation path Robert outlines in the documentation is to be used I guess an official patch to shared_count is necessary. Would be interesting to hear what the smart pointer experts think.
I agree - the silence is deafining. I know there are applications that use my smart pointer serialization code right now. I implemented and tested it but I don't really know for a fact that it addresses all aspects that need to be addressed such as exception safety, threading, and who knows what else.
Otherwise, I still think the solution I outlined in my previous review that doesn't require modifying shared_ptr could be considered. Downside being that it most likely adds a special-cased codepath to the serializer, but as I said I think it would be worth it.
I would hope that the developer's maintaining shared pointer will address this. putting special case code inside the serialization library would be a serious mistake. Between code, tests, and documentation, its approaching 30,000 lines.
The code is nicely written altough complex. Highly nested template code and lots of mpl. All compile-time checks and further factorizations are valuable.
Nitpick: serialization/scoped_ptr.hpp seems to have a typo. An extra #pragma once is present outside the #ifdef (_MSC..) block. It also includes serialization/pfto.hpp but doesn't seem to use it.
Thanks - I love picking these nits. Its easy and satisfying - really.
* Parts of the library that might become standalone boost libraries The library already provides good separation of components. tatic_warning, strong typedef, pfto, stack_allocate etc are all given candidates that would just need better documentation and (fast-track?) reviews.
A little bit more is going to be required. They've been tested in the context of serialization and that's OK. I think they need to be more polished and that's a little more work than it first appears. Also a separate review would probably fine tune their interface, implementation and capability. * What is your evaluation of the documentation?
I would also like to see an improved reference documentation that clearly states the requirements on the types/concepts involved.
Hmmm - that seems pretty unanimous - pretty much a first for boost as far as I know.
I support the separation of documentation (and implementation) of other boost components that like to support serialization. A simple index with links would suffice I think but most importantly a _consistent_ structure should be agreed upon which new components can follow.
I followed the boost document on how to write documentation. The only wrinkle I added was the right hand index/contents window. I'm pretty pleased with this. I would appreciate VC7.1 project files for the examples and tests though.
One of my tests added support for the STL container adaptors std::stack, queue and priority_queue. I think direct support for them could be convinient since they hide the native containers. I attach my implementation. Feel free to comment
I took a short look. The name stack_hack sort of begs some sort of comment. The main thing is that any component of this type has got to be submitted with a test. Thanks for taking the time to study the library and submit a review. Robert Ramey
From: "Robert Ramey"
Regarding shared_ & weak_ptr. If the implementation path Robert outlines in the documentation is to be used I guess an official patch to shared_count is necessary. Would be interesting to hear what the smart pointer experts think.
I agree - the silence is deafining. I know there are applications that use my smart pointer serialization code right now. I implemented and tested it but I don't really know for a fact that it addresses all aspects that need to be addressed such as exception safety, threading, and who knows what else.
Otherwise, I still think the solution I outlined in my previous review that doesn't require modifying shared_ptr could be considered. Downside being that it most likely adds a special-cased codepath to the serializer, but as I said I think it would be worth it.
I would hope that the developer's maintaining shared pointer will address this. putting special case code inside the serialization library would be a serious mistake. Between code, tests, and documentation, its approaching 30,000 lines.
FWIW, in my own library I do, indeed, have a special case code for serializing an unmodified shared_ptr, for two reasons. First, this shows that the library is flexible enough. Second, even if we modify boost::shared_ptr to be boost::serialization-friendly, we won't be able to patch std::tr1::shared_ptr. However, should you decide to use your current scheme, I'll happily insert whatever friend declarations are necessary in shared_count. I'm away from my computer right now, so I can't paste my read/write functions for shared_ptr, but the general idea is that they keep a map< shared_ptr<void>, int > on writing, mapping every shared_ptr ownership group to a pointer id, and a corresponding map< int, shared_ptr<void> > on reading, for the reverse transformation. The main problem for implementing a non-intrusive serialize() for shared_ptr is that these pointer maps need to be held in the archive, which requires either archive modification, or a general "extra state" support in all archives.
Peter Dimov wrote: [snip]
FWIW, in my own library I do, indeed, have a special case code for serializing an unmodified shared_ptr, for two reasons. First, this shows that the library is flexible enough. Second, even if we modify boost::shared_ptr to be boost::serialization-friendly, we won't be able to patch std::tr1::shared_ptr.
Interesting to hear that this approach has been used by more people! I also believe being able to handle almost any smart-ptr would be a big plus. Some monolitic frameworks "force" you to used their smart-ptrs for example... Do you use some kind of general registration mechanism for the classes that needs this treatment btw? [snip]
the general idea is that they keep a map< shared_ptr<void>, int > on writing, mapping every shared_ptr ownership group to a pointer id, and a corresponding map< int, shared_ptr<void> > on reading, for the reverse transformation.
The main problem for implementing a non-intrusive serialize() for shared_ptr is that these pointer maps need to be held in the archive, which requires either archive modification, or a general "extra state" support in all archives.
Hmm, in my implementation I _don't_ save anything but the raw-ptrs to the archive. Only at load-time is a temporary map maintained that intercepts loading of smart_ptrs and "seeds" them starting from the first one. Is the're something I'm missing with this approach? I guess such an implementation would be more complicated without a baseclass(?) (as in the CommonC++/MFC case). Otherwise I can only see how it allows less "strict" enforcement of load/save symmetry. I.e it would enable smart-ptr<->raw_ptr transfer. Bug or feature? ;-) Regards // Fredrik Blomqvist
From: "Fredrik Blomqvist"
Peter Dimov wrote: [snip]
FWIW, in my own library I do, indeed, have a special case code for serializing an unmodified shared_ptr, for two reasons. First, this shows that the library is flexible enough. Second, even if we modify boost::shared_ptr to be boost::serialization-friendly, we won't be able to patch std::tr1::shared_ptr.
Interesting to hear that this approach has been used by more people! I also believe being able to handle almost any smart-ptr would be a big plus. Some monolitic frameworks "force" you to used their smart-ptrs for example... Do you use some kind of general registration mechanism for the classes that needs this treatment btw?
I use a very simple manual registration scheme. Something like register_read<xml_reader, my_class>(); register_write<xml_writer, my_class>(); register_conversion<my_class, my_base>();
the general idea is that they keep a map< shared_ptr<void>, int > on writing, mapping every shared_ptr ownership group to a pointer id, and a corresponding map< int, shared_ptr<void> > on reading, for the reverse transformation.
The main problem for implementing a non-intrusive serialize() for shared_ptr is that these pointer maps need to be held in the archive, which requires either archive modification, or a general "extra state" support in all archives.
Hmm, in my implementation I _don't_ save anything but the raw-ptrs to the archive. Only at load-time is a temporary map maintained that intercepts loading of smart_ptrs and "seeds" them starting from the first one.
It's pretty much the same either way (assuming polymorphic classes and dynamic_cast<void*>(p)). The only case that comes to mind where the two approaches differ is when you save a shared_ptr<T>, then a shared_ptr<void> that shares ownership with it but points to a subobject. But this isn't a very important case in practice. I used a shared_ptr-specific map because I haven't even tried to handle raw pointers (by design) since a raw pointer can have so many meanings that - I decided - there was no reasonable default.
Peter Dimov wrote:
From: "Fredrik Blomqvist"
Peter Dimov wrote: [snip]
the general idea is that they keep a map< shared_ptr<void>, int > on writing, mapping every shared_ptr ownership group to a pointer id, and a corresponding map< int, shared_ptr<void> > on reading, for the reverse transformation.
The main problem for implementing a non-intrusive serialize() for shared_ptr is that these pointer maps need to be held in the archive, which requires either archive modification, or a general "extra state" support in all archives.
Hmm, in my implementation I _don't_ save anything but the raw-ptrs to the archive. Only at load-time is a temporary map maintained that intercepts loading of smart_ptrs and "seeds" them starting from the first one.
It's pretty much the same either way (assuming polymorphic classes and dynamic_cast<void*>(p)). The only case that comes to mind where the two approaches differ is when you save a shared_ptr<T>, then a shared_ptr<void> that shares ownership with it but points to a subobject. But this isn't a very important case in practice.
I used a shared_ptr-specific map because I haven't even tried to handle raw pointers (by design) since a raw pointer can have so many meanings that - I decided - there was no reasonable default.
Perhaps I'm being slow here, but I still don't understand why you need to save the extra map-information to the archive? What do you gain? Regardless of the raw-ptr issue, couldn't you simply treat any pointer in the archive as a shared_ptr then? // Fredrik
I believe all STL is handled. Serialization of other boost components will have to be the responsibility of their authors or others. Yes, of course. I didn't mean that it should be the serializer's responsibility to support all components. Just that _initially_ having a reasonable set of
Robert Ramey wrote: the most important components would greatly boost ;-) the libs usability from the end-users perspective.
There has been some discussion of where the serialization code for other boost libraries should reside. I don't have strong feelings about this other than I would prefer that it reside outside the serialization library itself. The only exception I would make is for the STL serialization which has no where else in boost to reside and it has a "special" status as and official C++ library.
I agree with this view.
Regarding shared_ & weak_ptr. If the implementation path Robert outlines in the documentation is to be used I guess an official patch to shared_count is necessary. Would be interesting to hear what the smart pointer experts think.
I agree - the silence is deafining. I know there are applications that use my smart pointer serialization code right now. I implemented and tested it but I don't really know for a fact that it addresses all aspects that need to be addressed such as exception safety, threading, and who knows what else.
I too am worried about possible showstoppers regarding thread-safety and such. More "boost-eyes" need to look at this IMO.
Otherwise, I still think the solution I outlined in my previous review that doesn't require modifying shared_ptr could be considered. Downside being that it most likely adds a special-cased codepath to the serializer, but as I said I think it would be worth it.
I would hope that the developer's maintaining shared pointer will address this. putting special case code inside the serialization library would be a serious mistake. Between code, tests, and documentation, its approaching 30,000 lines.
It's of course not a first-hand choice to add special cases, but on the other hand a construct for non-intrusive smart-ptr handling would be able to handle handle other _closed implementation_ pointers too.
* Parts of the library that might become standalone boost libraries The library already provides good separation of components. tatic_warning, strong typedef, pfto, stack_allocate etc are all given candidates that would just need better documentation and (fast-track?) reviews.
A little bit more is going to be required. They've been tested in the context of serialization and that's OK. I think they need to be more polished and that's a little more work than it first appears. Also a separate review would probably fine tune their interface, implementation and capability.
Yes, I don't think there's a need to rush it either. They could simply be "harvested" when the need & time allows.
* What is your evaluation of the documentation?
I would also like to see an improved reference documentation that clearly states the requirements on the types/concepts involved.
Hmmm - that seems pretty unanimous - pretty much a first for boost as far as I know.
Well, simple pre- and post-conditions and a spec about what concepts a type to be serialized has to conform to would cover most of it IMO. I also agree with Dave Abrahams view that this is something all libs should have and whether some miss it is more of a defect in that lib's documentation.
I support the separation of documentation (and implementation) of other boost components that like to support serialization. A simple index with links would suffice I think but most importantly a _consistent_ structure should be agreed upon which new components can follow.
I followed the boost document on how to write documentation. The only wrinkle I added was the right hand index/contents window. I'm pretty pleased with this.
Hmm, my point here wasn't about the main documentation but how an index containing an overview of which components from boost, STL etc have serialization capabilities could be built and linked to the main docs.
One of my tests added support for the STL container adaptors std::stack, queue and priority_queue. I think direct support for them could be convinient since they hide the native containers. I attach my implementation. Feel free to comment
I took a short look. The name stack_hack sort of begs some sort of comment. The main thing is that any component of this type has got to be submitted with a test.
Yes I understand that. I have some simpler tests but before I'd take the time to integrate with the full boost::test etc I wanted to see if the code was considered useful. The idea used in the impl. is that the std::container adaptors hide the adapted container behind a 'proteced' guard and can thus not take advantage of the built in serialization for stl-containers. My "hack" was to inherit from std::stack/queue to get access to the container and then forward it to the serializer (at the expense of one extra copying step). Also see the comments at the top of the .hpp files. In particular this turns the serialization of std::priority_queue into a O(n) operation since the heap interface is bypassed. Regards // Fredrik Blomqvist
participants (3)
-
Fredrik Blomqvist -
Peter Dimov -
Robert Ramey