Re: [boost] ptr_container: ptr_map_adapter interface

From: axter (boost_at_[hidden])
Date: 2006-01-02 20:19:54

"Thorsten Ottosen" <tottosen_at_[hidden]> wrote in message
news:<dpbv9p$k15$1_at_[hidden]>...
> axter wrote:
> > "Slawomir Lisznianski" <public_at_[hidden]> wrote in message 
> > news:<donook$li2$1_at_[hidden]>...
> > 
> >>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.