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

Subject: Re: [boost] [iterator] [property_map] Enhancement proposals
From: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2012-10-12 03:52:15

On Tue, Oct 9, 2012 at 4:37 PM, Louis Dionne <louis.dionne92_at_[hidden]>wrote:

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

Deal.

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

Wouldn't the first block of code above not compile regardless of whether
the unary constructor is a template constrained by enable_if<
is_convertible > or takes the wrapped iterator directly? The result of
std::back_inserter(vec) is not convertible to the intermediate
chained_output_iterator in either case.

Anyway, I think the typical uses would involve type deduction via
make_chained_output_iterator.

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

If you had the following chained_output_iterator constructor:

template< class G, class J >
chained_output_iterator(chained_output_iterator<G,J>,
  typename enable_if_c< mpl::and<
    is_convertible<H,G>,
    is_convertible<J,I>
  >::value >::type * = 0);

Oops, now chained_output_iterator< F, derived_t * > is convertible to
chained_output_iterator< F, base_t * >. is_convertible<J,I> is too loose
(admittedly, there are many iterator contexts where you simply cannot
prevent the derived_t * -> base_t * conversion, but if you can prevent it,
you should).

Presently, there is no such converting constructor, and I don't yet see a
need for one.

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

Ordinarily the reference typedef should be the result of operator*() const,
but I guess for output iterators the reference typedef is pretty
meaningless.

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

Best (i.e., least work for me) would be to provide the header, the unit
test, any patches to the present Boost.Iterator documentation, and a
contract signed in blood that you'll support this for life. Well, okay,
I'll take 3 out of 4 :)

Where you make the files available doesn't matter too much, just as long as
I can easily grab them when ready. I guess the official route is to create
a feature enhancement request trac ticket with the files attached.

Thanks,

- Jeff