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-08 22:56:05

On Mon, Oct 8, 2012 at 9:11 AM, Louis Dionne <louis.dionne92_at_[hidden]>wrote:

> Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung <at> gmail.com> writes:
>
> > Regarding accessor_iterator: The implementation is very short, but it
> still
> > seems to have minimal if any advantage to just using a transform_iterator
> > directly composed with a function object wrapping the pointer-to-member
> > (either at runtime or compile-time [1]). Also, seems you're lacking a
> > convenient make_accessor_iterator (which, of course, would probably have
> to
> > be a macro, as described in [1]).
>
> Regarding the make_accessor_iterator:
> I did not provide such a function because I did not know how to implement
> it.
> After reading the link, I indeed see how it could be done. I will do it if
> the
> class is accepted.
>

Yeah, it is tricky, and the referenced solution is pretty clever :)

> Another possibility is to set the pointer to member at runtime, which then
> makes the implementation of make_accessor_iterator trivial. You can see
> this
> alternative implementation on the dynamic_accessor_iterator branch of the
> git
> repository.
>

Hopefully you'll excuse me for making an educated guess on what the
implementation would look like.

> Regarding the usefulness of the class vs using transform iterator:
> I too believe the same thing can be achieved using a transform_iterator. It
> really depends on whether you think creating a function object is a pain.
> It
> would be useful to know what others think of it to know whether there is a
> need in the community.
>

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
community pushes for including these variants directly in Boost.Iterator,
that's fine, but I'm disinclined to do so otherwise given the simplicity of
combining transform_iterator with the appropriate function object. Further,
I don't consider the creation and use of the appropriate function objects
to be a pain, given the present solutions within Boost.

> This could be useful, but there's a few things I need to understand about
> > your implementation first.
> > - 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.

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

> > - I'm wondering why the increment of the wrapped iterator happens inside
> > proxy::operator= rather than chained_output_iterator::operator++...?
> Also,
>
> You are right, this is my mistake and it's fixed on master now.
>
> Since this change would make the implementation with
> function_output_iterator
> more complicated than the `from scratch' implementation, I believe
> function_output_iterator should not be used.
>
> Note: The implementation using function_output_iterator comes from a
>       suggestion in your first reply and is available on the
>       using_function_output branch of the repository.
>

Hmmm, yeah I don't recall anything involving function_output_iterator, I
must've missed something...

> > typically operator* is a const member function, and moving the
> advancement
> > of the wrapped iterator into chained_output_iterator::operator++ would
> > allow that. I might be missing something though.
>
> In order to make the chained_output_iterator::operator* const, we would
> need
> to make the proxy hold copies or const references to the wrapped iterator
> and
> function. However, using const references would add the following
> restrictions:
>     - the wrapped iterator's operator* must be const
>

The typical case...

>     - the function object's operator() must be const
>

Again, the typical case. As a general rule of thumb, you have to be careful
with state-mutating function objects immediately held within iterators, but
I guess strictly incrementable iterators (as chained_output_iterator is)
have more predictable access patterns so it's less of a concern.

> For these reasons, I believe the proxy should either store copies or we
> should
> give up making chained_output_iterator::operator* const. I'll wait for your
> reply before I change that.
>

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

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

- Jeff