[boost] [ptr_container] post review comments

From: Pavel Chikulaev (pavel.chikulaev_at_[hidden])
Date: 2005-04-07 09:28:36

Thorsten,

I've got some comments on ptr_container library:

1. Since we can iterate over elements in container using pair of begin end or
pair of ptr_begin ptr_end it's not enough to provide member-functions which
take only iterators as arguments, we also need overloads which take
ptr_iterators.

Motivation example:

ptr_vector<int> v;
//...
ptr_vector<int>::iterator i = std::find(v.begin(), v.end(), 5);
if (i != v.end())
    v.erase(i); //works just fine, but

int k = 5;
int * pk = &k;
ptr_vector<int>::ptr_iterator i = std::find(v.ptr_begin(), v.ptr_end(), pk);
if (i != v.ptr_end())
    //here we can just write simple erase, we need to write something like that:
    v.erase(v.begin() + std::distance(v.ptr_begin(), i));
    //and it's really no good.

I'd like to list all the functions that need to have a twin, but I'd better say
shorter - at least every public function that have one or more parameters
with type iterator. Differently, ptr_iterators seem to be sick, petty and
harmed. We just can't use them anywhere.

Another, more simple approach is to provide function that returns iterator
of the corresponding ptr_iterator. The funny thing is that we already can
get ptr_iterator from iterator, but it's not so needed as the backward
transformation. But I do think this is not a good solution, because with this
approach we can achive the required functionality, but the syntax will be
very very bad.

One more approach is to simply get rid of ptr_iterators, but provide user
with adapters he will ever need. I like it, but sometimes (or more oftener)
I (and you) still simply need ptr_iterators.

IMO the best solution is the hardest solution - approach number 1.

2. The next drill is on templated functions and constructors.
    If we will decide to keep ptr_iterators, consider the following code:

    ptr_vector<int> v;
    ptr_vector<int> v2(v.ptr_begin(), v.ptr_end());

    IMO user (that was I) expects this compiling. But it's not.
    Why can't I construct ptr_vector from ptr_iterators?
    It's good iterators, just any else, but I can, because they need
    special care to handle them.
    IMO it's nonsense not to enable such support.
    The same applies to templated insert, assign, and other functions.
    So, all of them need an overload for templated ptr_iterators.

    I think the library should either supply the user of ptr_iterators
    all he will every need or get rid of them completely.

No some more remarks, but they are not so important as the above.

4.  IMO header <boost/ptr_container/clone_allocator.hpp>
    deserves to be higher in folder hierarchy since its' contents
    can be widely used by other libraries/people.

5. IMO header <boost/ptr_container/exception.hpp> contents doesn't follow good
    naming scheme.
        What do other people think when they see boost::bad_index & ?
    Some class, maybe exception, since it has prefix bad, but how knows
    that this class comes with ptr_container library? No-one. BTW
    why don't you use std::out_of_range? At least please change its' name.
    The same for boost::bad_pointer in the same header file.

6. What a funny name!!
    boost::ptr_container_ptr_container_detail in
<boost/ptr_container/nullable.hpp>
    Maybe this header also deserves more fame(I mean nullable<T>)?

7. Why don't you throw std::out_of_range exception like std containers when
    ptr_sequence_adapter::at is out of bounds? Instead, you throw unknown
    boost::bad_index.

8. It's seems you didn't put contents of detail headers in detail namespace so
    we have boost::map_config class in
<boost/ptr_container/ptr_map_adapter.hpp>.
    Why don't you put all adapters and their config classes to
ptr_container::detail
    namespace? I don't think anyone will use them.

9. <boost/ptr_container/ptr_predicate.hpp> contents derserve to be documented.
    And it's better to make ptr versions to all <functional> functions and
    introduced these classes:
        template<typename T, typename U, typename R>
        class boost::ptr_binary_function : std::binary_funtion<T *, U *, R> {};
        template<typename T, typename R>
        class boost::ptr_unary_function : std::unary_funtion<T *, R> {};
    It will simplify your code and IMO it will be useful for users who wants
    to make their own ptr functions/predicates.

10. indirected1, indirected2 are not good names, don't you think?

11. std::set doesn't have set::at, why does boost::ptr_set have one(even two)?

Sorry to dig so deep.

P.S. BTW, have you fixed that const_ptr_iterator bug yet?

--
Pavel Chikulaev