[ptr_container] post review comments

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
participants (1)
-
Pavel Chikulaev