Hi Pavel, Finally someone who comments :-) "Pavel Chikulaev" <pavel.chikulaev@gmail.com> wrote in message news:d33fum$rdh$1@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