Re: [boost] [iterator] [property_map] Enhancement proposals

Subject: Re: [boost] [iterator] [property_map] Enhancement proposals
From: Louis Dionne (louis.dionne92_at_[hidden])
Date: 2012-10-09 19:37:07

Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung <at> gmail.com> writes:

> It's just unclear to me where one should draw the line as far as what
> iterators should be provided by Boost.Iterator out of the box. Neither your
> original accessor_iterator nor dynamic_accessor_iterator cover what I would
> imagine to be the equally common case of applying a nullary member function
> to an iterator's value. So I can easily imagine 6 different variants of
> your accessor_iterator: { static, dynamic } x { member data pointer,
> non-const member function pointer, const member function pointer }. If the
[...]

I had not thought about it that way initially but I agree with you. So let's
forget about the accessor_iterator. Since the code won't go anywhere, it can
always be added later if the community asks for it.

> > > - Using is_convertible to determine iterator convertibility is, strictly
> > > speaking, too loose, as it allows derived* -> base* conversions that are
> > > undesirable if sizeof( derived ) > sizeof( base ). At some point I
> > probably
> > > need to fish through the Iterator library and change any such uses
> > myself...
> >
> > Given the 3 places where I use it currently:
> >
> >     template <typename OutIter>
> >     explicit chained_output_iterator(Function const&, OutIter const&,
> >         typename enable_if_convertible<OutIter, OutputIterator>::type* =0);
> >
> >     template <typename OutIter>
> >     chained_output_iterator(OutIter const&,
> >         typename enable_if_convertible<OutIter, OutputIterator>::type* =0);
> >
> >     template <typename Func, typename OutIter>
> >     chained_output_iterator(chained_output_iterator<Func, OutIter> const&,
> >         typename enable_if_convertible<Func, Function>::type* =0,
> >         typename enable_if_convertible<OutIter, OutputIterator>::type* =0);
> >
> > I take it that you are referring to the first two copy constructors being
> > too loose on the `OutIter' type. Would the following be alright, given
> > that OutputIterator is the type of the wrapped iterator?
> >
> >    explicit chained_output_iterator(Function const&,OutputIterator const&);
> >    chained_output_iterator(OutputIterator const&);
>
> Sorry, I had another (albeit related) concern in mind. Yes, I think it's
> simpler to just make the constructor parameters the wrapped iterator type
> directly.

Accepting any iterator that is convertible to the wrapped iterator is nice
if it is not dangerous because it allows to do this:

    typedef chained_output_iterator<Functor1,
                chained_output_iterator<Functor2,
                    std::back_insert_iterator<std::vector<T>>>> OutIter;
    // Then, if Functor1 and Functor2 are default constructible:
    std::vector<T> vec;
    OutIter result(std::back_inserter(vec));

If the constructor only takes an instance of the wrapped iterator, the
following must be done, which could be annoying if one creates a long chain:

    typedef chained_output_iterator<Functor2,
                std::back_insert_iterator<std::vector<T>>> Chain;
    typedef chained_output_iterator<Functor1, Chain> OutIter;
    std::vector<T> vec;
    OutIter result(Chain(std::back_inserter(vec)));

So I would advocate in favor of keeping the initial implementation for the
first two constructors, unless I am missing something.

> The instances where one needs to be a bit careful are converting
> constructors from one chained_output_iterator to another.

I don't see how this could be a problem. I don't understand in which
circumstances an undesirable derived* -> base* conversion could happen.
In all cases, I'll stick to a regular copy constructor unless there is a
better solution.

> You can always provide the best of both by providing both const and
> non-const overloads of operator* (and likewise "const" and "non-const"
> proxy classes).

Good idea, I just did that.

> I think I see chained_function_output_iterator as a viable addition to
> Boost.Iterator; accessor_iterator, I'm yet to be convinced. Nothing
> personal :)

I'm glad to read that, and I agree with your for the accessor_iterator. After
the last questions are settled for the chained_output_iterator, I will write
more unit tests, examples and documentation for the class. Do you want me to
make everything available on GitHub so you can add it to the library, or is
there another preferred method?

Louis Dionne