[boost] Re: [ptr_container] post review comments

From: Pavel Chikulaev (pavel.chikulaev_at_[hidden])
Date: 2005-04-07 14:39:15

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

>
> | 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. Because, on the one hand, they are iterators of elements in
    container, but we can't make a copy of container using these iterators,
    and we just can't use them almost anywhere, on the other hand, if we
    provide them with functions we can put them in, changes in std algorithms
    are also needed to unify usage, but since we can't do it, there is no
    place for ptr_iterators at all. We can always use lambda:
        int * p;
        std::find_if(v.begin(), v.end(), &_1 ==  p);

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

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

It's not needed anymore.

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

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

    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)

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

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

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

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

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

--
Pavel Chikulaev