Re: [boost] ptr_container: ptr_map_adapter interface
"Thorsten Ottosen" <tottosen@dezide.com> wrote in message news:<dpbv9p$k15$1@sea.gmane.org>...
axter wrote:
"Slawomir Lisznianski" <public@paramay.com> wrote in message news:<donook$li2$1@sea.gmane.org>...
Hello,
I've been using ptr_map lately and have a few questions regarding its interface.
Below is a signature of `insert' function as declared in ptr_map_adapter:
std::pair<iterator,bool> insert( key_type& k, value_type x );
Why is `k' a non-const reference?
it's a matter about exception-safety.
see the FAQ.
To imply ownership transfer, couldn't `x' be of `std::auto_ptr<value_type>' type instead?
right. the next release will have overloads taking auto_ptr<T> as well as T*.
What happend to std::pair as an argument of insert? Was symmetry with std::map dropped for a reason here?
yes, exception-safety.
From my test, it doesn't seem as though ptr_map has consistent and reliable implementation.
Do you care to explain in detail what that means?
One of the reasons to use pointers, is so that you don't have object slicing. ptr_map is a container of pointers, but you still get object slicing. IMHO, that inconsistent at best, and unreliable at worse.
I recommend that boost replace the pointer containers with the following smart pointers: http://code.axter.com/copy_ptr.h http://code.axter.com/cow_ptr.h
Example usage: std::map<int, cow_ptr<foo> > MyPtrMap;
The above type gives you more functionality then the boost::ptr_map type, and of course, it's interface matches the std::map interface. Where as the boost::ptr_map type has the following problems: 1. Doesn't allow you to use an abstract pointer
Really?
2. Requires a non-constant argument for the insert function
Right, to give exception-safety.
3. Requires value semantics for the operator[] function
?
4. When assigning value via operator[] from one container to another, you get object splicing for derived types
So?
See above comment:
5. It doesn't seem to compile on VC++ 6.0, and when it compiles on VC++ 7.1 you get compiler warnings
Yes, vc7.1 warns about a lot of things, particular ADL. Turn it off.
If you use cow_ptr instead, you don't have the above problems.
There are similar problems in the boost::ptr_set class, which also can be replaced with cow_ptr or copy_ptr std::set<cow_ptr<foo> > MyPtrSet;
IMHO, the entire boost pointer container set of classes can be replace with either cow_ptr and/or copy_ptr, and the result would be a more reliable,
reliable in what way?
Reliable in that you get expected interface when compiling. Reliable in that you get expected behavior at runtime.
Switching to a deep-copying smart pointer can have significant performance implications, particularly (but not only) with compilers without RTO, like vc6.
VC6 doesn't even compile using the boost::ptr_map or boost::ptr_set?..... You can eliminate performance implications by using a deep-copying smart pointer like the cow_ptr, which I previously posted. Moreover, I wouldn't be surprise if the cow_ptr was able to out perform the boost pointer containers. I'm currently working on making a comparison between the boost pointer containers and copy_ptr/cow_ptr. I also plan to do performance test, so that the comparison will be more complete. Below I list some of the issues found using the boost pointer containers. Some of the issues are by design, and some are not. Whether it's by design or not, I still consider the issues relevant in a comparison. The test was performed using boost_1_33_0 and VC++ 7.1, 6.0, and GNU 3.x. You can download the test code via following link: http://code.axter.com/BoostPtrContainerTestCode.zip The current test code is a little hard to read, but once I'm completely done with all the test, I will clean it up. Please feel free to correct me on any of the items listed below. Most of the items listed below, you'll find relevant comments in the above test code. Boost::ptr_vector 1. The constructor that takes two iterators fails to compile on VC++ 6.0 2. Fails to include a copy constructor (by design) 3. ptr_vector::push_back fails to compile with a de-referenced iterator 4. ptr_vector produces a runtime error when assigning one container to another via operator[] 5. Produces multiple compiler warnings when using VC++ 7.1 6. Will fail to clone the right type if a derived-derived type fails to implement the clone method. 7. Does not have the standard vector::assign member function that takes (size_type, const Type&) boost::ptr_set 1. Fails to compile on VC++ 6.0 2. Can not insert using an abstract pointer. 3. Can not find using an abstract pointer. 4. Crashes when inserting via de-referenced iterator from another container 5. boost::ptr_set is publicly derived from boost::ptr_set_adapter, however ptr_set_adapter does not have a virtual function, nor does any of it's base classes. 6. Fails to compile when used with ptr_set::const_reverse_iterator logic boost::ptr_map 1. Unable to compile with an abstract type 2. Does not have an insert type for std::pair 3. Can not insert using a constant for 1st argument 4. Unable to compile on VC++ 6.0 5. Does not support the pointer as the key type 6. Fails to compile insert to iterator type 7. ptr_map::equal_range does not return std::pair<iterator, iterator> type as does the standard map::equal_range 8. Does not use the standard first and second iterator members as does the standard map::iterator 9. Instead of using first and second iterator members, boost::ptr_map uses operator-> and key() functions to access first and second. However, it still fails to compile when using const_reverse_iterator.
axter wrote:
"Thorsten Ottosen" <tottosen@dezide.com> wrote in message news:<dpbv9p$k15$1@sea.gmane.org>...
axter wrote:
From my test, it doesn't seem as though ptr_map has consistent and
reliable
implementation.
Do you care to explain in detail what that means?
One of the reasons to use pointers, is so that you don't have object slicing. ptr_map is a container of pointers, but you still get object slicing. IMHO, that inconsistent at best, and unreliable at worse.
4. When assigning value via operator[] from one container to another, you get object splicing for derived types
So?
See above comment:
This is your own fault....you should make your class hierarchy noncopyable by default. If you really want to assign the pointers, then use replace().
5. It doesn't seem to compile on VC++ 6.0, and when it compiles on VC++ 7.1 you get compiler warnings
Yes, vc7.1 warns about a lot of things, particular ADL. Turn it off.
If you use cow_ptr instead, you don't have the above problems.
There are similar problems in the boost::ptr_set class, which also can be replaced with cow_ptr or copy_ptr std::set<cow_ptr<foo> > MyPtrSet;
IMHO, the entire boost pointer container set of classes can be replace with either cow_ptr and/or copy_ptr, and the result would be a more reliable,
reliable in what way?
Reliable in that you get expected interface when compiling. Reliable in that you get expected behavior at runtime.
I think you would use the term "familiar" here. Reliability for me implies safety or performance guarantees.
Switching to a deep-copying smart pointer can have significant performance implications, particularly (but not only) with compilers without RTO, like vc6.
VC6 doesn't even compile using the boost::ptr_map or boost::ptr_set?.....
well, it's basically an ancient compiler and I don't have the resources to port to it anymore.
You can eliminate performance implications by using a deep-copying smart pointer like the cow_ptr, which I previously posted. Moreover, I wouldn't be surprise if the cow_ptr was able to out perform the boost pointer containers.
I would. COW doesn't have a good reputation.
I'm currently working on making a comparison between the boost pointer containers and copy_ptr/cow_ptr. I also plan to do performance test, so that the comparison will be more complete.
Ok, for what its worth, I'm looking forward to reading it. You might consider a test comparing the binary size too.
Below I list some of the issues found using the boost pointer containers. Some of the issues are by design, and some are not. Whether it's by design or not, I still consider the issues relevant in a comparison. The test was performed using boost_1_33_0 and VC++ 7.1, 6.0, and GNU 3.x. You can download the test code via following link: http://code.axter.com/BoostPtrContainerTestCode.zip The current test code is a little hard to read, but once I'm completely done with all the test, I will clean it up.
Please feel free to correct me on any of the items listed below. Most of the items listed below, you'll find relevant comments in the above test code.
I will comment on these later tonight. -Thorsten
axter wrote:
Please feel free to correct me on any of the items listed below. Most of the items listed below, you'll find relevant comments in the above test code.
3. ptr_vector::push_back fails to compile with a de-referenced iterator
I originally had this, but during the review there was some objection against having these.
4. ptr_vector produces a runtime error when assigning one container to another via operator[]
I need to see an as small as possible test that shows this. Given that you prohibit slicing (by declaring an explicit copy constructor or by removing copyability totally) it might be due to this.
5. Produces multiple compiler warnings when using VC++ 7.1
Yes. Vc7.1 and other compilers warn against far too much. ADL for example.
6. Will fail to clone the right type if a derived-derived type fails to implement the clone method.
There is no magic like in shared_ptr here. That would impose some overhead.
7. Does not have the standard vector::assign member function that takes (size_type, const Type&)
Might be possible to add, though the second argument might be an auto_ptr, a bald pointer or a reference. I'm considering adding a little more initialization help in boost.assign: http://www.boost.org/libs/assign/doc/index.html#ptr_push_back
2. Can not insert using an abstract pointer.
implement new_clone().
3. Can not find using an abstract pointer.
please produce a minimal example after implmenting new_clone().
4. Crashes when inserting via de-referenced iterator from another container
ditto.
5. boost::ptr_set is publicly derived from boost::ptr_set_adapter, however ptr_set_adapter does not have a virtual function, nor does any of it's base classes.
The classes are not copyable, so you won't run into slicing problems. The inheritance is purely for inheriting implementation.
6. Fails to compile when used with ptr_set::const_reverse_iterator logic
please produce a minimal example.
boost::ptr_map 1. Unable to compile with an abstract type
implement new_clone();
2. Does not have an insert type for std::pair
and cannot provide one without allowing memory leaks.
3. Can not insert using a constant for 1st argument
ditto.
5. Does not support the pointer as the key type
right. use shared_ptr or something then.
6. Fails to compile insert to iterator type
please produce minimal example.
7. ptr_map::equal_range does not return std::pair<iterator, iterator> type as does the standard map::equal_range
nor does it need to. the two classes are used in vastly different domains and so generic code working with them both is an illusion.
8. Does not use the standard first and second iterator members as does the standard map::iterator
right, you're not allowed access to the pointer.
9. Instead of using first and second iterator members, boost::ptr_map uses operator-> and key() functions to access first and second. However, it still fails to compile when using const_reverse_iterator.
A minimal example is appreciated. Thanks Thorsten
participants (2)
-
axter -
Thorsten Ottosen