[boost] Re: [ptr_container] post review comments

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2005-04-07 15:11:01

"Pavel Chikulaev" <pavel.chikulaev_at_[hidden]> wrote in message 
news:d34249$jo$1_at_sea.gmane.org...
| "Thorsten Ottosen" <nesotto_at_[hidden]> wrote in message
| news:d33saa$bn2$1_at_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