[boost] Re: [ptr_container] post review comments

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2005-04-07 12:54:20

Hi Pavel,

Finally someone who comments :-)

"Pavel Chikulaev" <pavel.chikulaev_at_[hidden]> wrote in message
news:d33fum$rdh$1_at_sea.gmane.org...
| Thorsten,
|
| 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.

well, you can't normally make a search like that unless you use the view-clone
allocator.

| 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.

the intention was that they could be used with permutating, mutating standard
algorithms,
eg. sort(). you need access to the pointer to be able to do that.

| 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.

well, the function that currently turn a ptr_iterator into an iterator is
called the constructor of an iterator.

As you say, ptr_iterators seems to be sick...in fact, the only legal
solution is to simply let them be iterators over void*'s. yes, the current
implementation might work on 99% of all compilers, but it is not
guaranteed to do so.

So that has to happen. Given that, do we still want them? The alternatives
are

1. yes, provide them and let users do the cast inside their functor

2. no, don't provide them

2. no, don't provide them, but implement safe versions of the most common
    algorithms, eg.

sort
unique
merge
erase
erase_if

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

well, let's dezide that first, then.

| 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.

I'll consider this.

| 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?

    because it takes an std::string argument in its constructor.

| At least please change its' name.
|    The same for boost::bad_pointer in the same header file.

isn't the important part that we inherit from std::exception?

what names do you suggest?

| 6. What a funny name!!
|    boost::ptr_container_ptr_container_detail in
| <boost/ptr_container/nullable.hpp>

ah, a search and replace bug.

|    Maybe this header also deserves more fame(I mean nullable<T>)?

perhaps.

| 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.

because of the string argument

| 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.

it was requested during the review. I'm not too keen on the idea, but
it doesn't hurt presently.

I think you're right that map_config and ptr_map_adapter_base ect should
go into ptr_container_detail namespace.

| 9. <boost/ptr_container/ptr_predicate.hpp> contents derserve to be
documented.

actually, it should be removed.

|    And it's better to make ptr versions to all <functional> functions and
add
| 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?

please take a look at indirect_fun; it is meant to replace the ptr_predicate
header.

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

I haven't documented it yet because I was doubtful about it too. I all for
removing it.

| Sorry to dig so deep.

no, thank you for digging so deep.

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

about conversion from ptr_iterator? If so yes,

-Thorsten