"Pavel Chikulaev" <pavel.chikulaev@gmail.com> wrote in message news:d34249$jo$1@sea.gmane.org... | "Thorsten Ottosen" <nesotto@cs.auc.dk> wrote in message | news:d33saa$bn2$1@sea.gmane.org... | > well, you can't normally make a search like that unless you use the view-clone | > allocator. | | What you mean? | if we have erase erase(ptr_iterator), shouldn't erase(i) work properly | despite | current allocator? yes, but I was commenting the search for the address of an element on the stack. | > | > | 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. | | I see. The more I think about it, the more I think that we should remove | ptr_iterators. I have a similar feeling. | > | 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 | | I vote for this. | > | > 2. no, don't provide them, but implement safe versions of the most common | > algorithms, eg. | > | > sort | > unique | > merge | > erase | > erase_if | > you don't want the algorithms? | > | 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? | First, I suggest putting everything in ptr_container namespace. I meant the name of the exceptions. | Second, classes that are not likely going to be used by other | people put in ptr_container::namespace. | And in each header <boost/ptr_container/ptr_X.hpp> promote | X to boost namespace. (Where X are one of these: vector, list, | deque, map, set, etc) | | Also I'd like to tell you what I think about namespaces. | Most people (and it seems you too) think that if they put their | classes in namespace, no problems with namespaces is ever | going to be happed. But it's not. In our example the namespace | is called 'boost'. So if every boost developer will put their classes | just in namespace 'boost' without structurizing it would be same | problems as C has with names. In your case, everything should | be in ptr_container namespace, and only the top of the iceberg | should be above water, in namespace boost. yeah, well, there is not going to many other ptr_vector<T> in boost. | > | Maybe this header also deserves more fame(I mean nullable<T>)? | > | > perhaps. | | Now I think it doesn't deserve any. It serves only one serve: to tell the | pointer | container that null pointers are not error. | And we have optional<T> (at least in runtime) nullable<T> was the best compromize I could find for the behavior. Adding another policy seemed overkill. optional<T> has some parallels, but it would not have the same meaning if we wrote ptr_vector< optional<T> >. ptr_vector<nullable<T>> is something similar to vector<optional<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. | > | > because of the string argument | About bad_index: | What string argument? You have same char *. Even if you make ctor with | std::string, and if you user catchs only std::exception, he is going to | get | information using const char * what(), so std::string is not needed | here. | So, I don't see ANY reason not to use std::out_of_range! the copy-constructor of std::string might throw; hence when constructing the bad_index exception-object, we might not get that far and the *wrong* error will be reported. | About bad_pointer: | It's better to derive from std::runtime_error or even throw | std::runtime_error("ptr_container: null pointer not allowed"); | You don't ever throw bad_ptr_container_operation, do you? | (BTW, maybe it should be better called ptr_container::bad_operation? | because your ptr_containers are not so bad :)) | | I think you shouldn't introduce and use any own classes derived from | std::exception, because your containers are only extension to std containers | that simplifies using of pointers in containers. somebody might want to catch a more specific exception. | So everybody expects it | to work as same as std::containers when possible. | It applies not only to exceptions but all member functions and so on. | Example from std: Everybody expects(expected) vector<bool> to be same | as any other vector<T> specialization, but it is not. Now, IFAIK almost | nobody use it, and I really don't want your library to repeat vector<bool> | failure. I don't think the analogy is quite the same. | > | 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. | | If I were you, I even go further - I would put everything in detail | namespace | except ptr_XXXX classes and allocators. yeah, maybe. I'll think about it. | > | > | 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? | | Since their operators () return bool we can call them | indirected_unary_predicate and indirected_binary_predicate. | But since we have indirect_fun we better remove them all. | | > please take a look at indirect_fun; it is meant to replace the ptr_predicate | > header. | | I've already taken a look, but I simply had no comments about it. | But now I do. | | BTW indirect_fun is needed only if user uses ptr_iterator, but with | their current capabilities, user won't use neither ptr_begin-ptr_end nor | indirect_fun. that is true. When/If I remove ptr_iterator, this header could be kept an implementation detail. | Ex: | std::sort(v.ptr_begin(), v.ptr_end(), indirect_fun<greater<int> >()); | std::sort(v.begin(), v.end(), greater<int>()); | IMO it's better to use lambda: | std::sort(v.ptr_begin(), v.ptr_end(), *_1 > *_2); | Don't you think this syntax is much more readable/flexible/extensible | and so on? I just don't see advantages of indirect_fun, may be you do? lambda works ok when you have a simple function like <. Some people like the other approach I guess. | > | 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. | | Looking forward to it. BTW what was the reason you added them? maybe because they are in map, but thinking about them, I don't think they make much sense in ptr_set. | > | P.S. BTW, have you fixed that const_ptr_iterator bug yet? | > about conversion from ptr_iterator? If so yes, | | Thanks, can you upload a fixed version to sandbox? The main-cvs version is somewhat newer; you can access that one, right? br -Thorsten