On Mon, Oct 8, 2012 at 9:11 AM, Louis Dionne <louis.dionne92@gmail.com>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.
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
This could be useful, but there's a few things I need to understand about 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