[gil] Is bit_aligned_pixel_iterator a standard conformant random access iterator?
Hi there, I'm having an interesting discussion over at the MSDN forums regarding compiler errors with MSVC10 that show up when using standard algorithms and bit_aligned images. Basically the question is why MSVC10 doesn't understand that bit_aligned_pixel_iterator is a random access iterator. For some reasons it thinks it's an input iterator and though std::copy, in this case, cannot find the correct copy implementation. One of the folks over at MSDN thinks that bit_aligned_pixel_iterator isn't implemented correctly with the interator_facade. He/She also thinks that iterator isn't fully standard compliant since it doesn't give a real reference but a proxy object instead when deferencing. I'm not sure if that's correct. Anyone knows if that's true or not. Here is the full text: http://social.msdn.microsoft.com/Forums/en-US/vcgeneral/thread/3c9eac4f-86e4... In any event it seems I need to do some adjustment in the the gil code to have it working with my gil io extension and MSVC10. Right now I have the compiler errors all over my test suite. Regards, Christian
At Sun, 30 May 2010 16:27:29 -1000, Christian Henning wrote:
Hi there, I'm having an interesting discussion over at the MSDN forums regarding compiler errors with MSVC10 that show up when using standard algorithms and bit_aligned images. Basically the question is why MSVC10 doesn't understand that bit_aligned_pixel_iterator is a random access iterator. For some reasons it thinks it's an input iterator and though std::copy, in this case, cannot find the correct copy implementation.
One of the folks over at MSDN thinks that bit_aligned_pixel_iterator isn't implemented correctly with the interator_facade. He/She also thinks that iterator isn't fully standard compliant since it doesn't give a real reference but a proxy object instead when deferencing. I'm not sure if that's correct. Anyone knows if that's true or not.
It is indeed correct that ForwardIterator (and thus RandomAccessIterator) forbids the use of proxy references. That restriction is supposed to be dropped in C++0x. -- Dave Abrahams Meet me at BoostCon: http://www.boostcon.com BoostPro Computing http://www.boostpro.com
On 5/31/10 5:33 AM, "David Abrahams" <dave@boostpro.com> wrote:
At Sun, 30 May 2010 16:27:29 -1000, Christian Henning wrote:
One of the folks over at MSDN thinks that bit_aligned_pixel_iterator isn't implemented correctly with the interator_facade. He/She also thinks that iterator isn't fully standard compliant since it doesn't give a real reference but a proxy object instead when deferencing. I'm not sure if that's correct. Anyone knows if that's true or not.
It is indeed correct that ForwardIterator (and thus RandomAccessIterator) forbids the use of proxy references. That restriction is supposed to be dropped in C++0x.
That is unfortunate because in many places in GIL we require a proxy reference - such as dereferencing a planar pixel, for example. I don't see a way to avoid having proxy references... And we do want GIL iterators to be random access iterators. Lubomir
At Mon, 31 May 2010 12:09:56 -0700, Lubomir Bourdev wrote:
On 5/31/10 5:33 AM, "David Abrahams" <dave@boostpro.com> wrote:
At Sun, 30 May 2010 16:27:29 -1000, Christian Henning wrote:
One of the folks over at MSDN thinks that bit_aligned_pixel_iterator isn't implemented correctly with the interator_facade. He/She also thinks that iterator isn't fully standard compliant since it doesn't give a real reference but a proxy object instead when deferencing. I'm not sure if that's correct. Anyone knows if that's true or not.
It is indeed correct that ForwardIterator (and thus RandomAccessIterator) forbids the use of proxy references. That restriction is supposed to be dropped in C++0x.
That is unfortunate because in many places in GIL we require a proxy reference - such as dereferencing a planar pixel, for example. I don't see a way to avoid having proxy references... And we do want GIL iterators to be random access iterators.
Well fortunately, *most* standard libraries have done *some* work to accomodate such iterators in their algorithms. This might be a good reason to duplicate some of the STL's algorithms in boost::algorithm. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
Hi all, On Mon, May 31, 2010 at 2:33 AM, David Abrahams <dave@boostpro.com> wrote:
It is indeed correct that ForwardIterator (and thus RandomAccessIterator) forbids the use of proxy references. That restriction is supposed to be dropped in C++0x.
Well, at least there is light and the end of the tunnel. ;-) As Lubomir has pointed out some iterators need proxy references. These reference proxy act like a real reference. So if it walks like a duck, .... Or am I missing something here? Regards, Christian
Hi Christian, this is ildjarn from the MSDN forums. :-] Christian Henning <chhenning <at> gmail.com> writes:
Hi there, I'm having an interesting discussion over at the MSDN forums regarding compiler errors with MSVC10 that show up when using standard algorithms and bit_aligned images. Basically the question is why MSVC10 doesn't understand that bit_aligned_pixel_iterator is a random access iterator.
To be clear, it's not that MSVC10 doesn't understand that bit_aligned_pixel_iterator is a random access iterator, it's that bit_aligned_pixel_iterator *isn't* a random access iterator. I.e., std::iterator_traits< bit_aligned_pixel_iterator >::iterator_category is std::input_iterator_tag on every compiler/platform. It's just that MSVC10 refuses to instantiate std::copy given a destination iterator categorized as an input iterator (which is a good thing). If GIL had a regression test with a static assertion that boost::is_same< std::iterator_traits< bit_aligned_pixel_iterator<...> >::iterator_category, std::random_access_iterator_tag >::value == true, this would have been caught long ago. (hint, hint) One perspective is that the bug here is that bit_aligned_pixel_iterator is trying to pass itself off as a random access iterator in the first place, despite the fact that it returns a proxy object rather than an actual reference, violating the standard's requirements for forward iterators. However, since it appears to work properly on all tested toolsets, that could be deemed an acceptable flaw, in which case the proper fix is to pass iterator_facade a category of std::random_access_iterator_tag rather than boost::random_access_traversal_tag. Easy enough. :-] Another perspective is that the bug here is caused by iterator_facade silently decaying into an input iterator despite being told to function as a random access iterator. This is documented behavior, so it's not a bug in and of itself, but the fact that it does so *silently* is a questionable design issue given that it's negatively affected at least two released libraries so far. I'm not sure what mechanism could be used to make the decay 'noisy' rather than silent, but this is clearly a gotcha for users of iterator_facade. Also, the fact that it decays into an input iterator rather than an input/output iterator seems arbitrary and less than ideal. Why doesn't it decay into boost::detail::input_output_iterator_tag? At least then it could be used as a destination for std::copy. Keep in mind that the issue surrounding bit_aligned_pixel_iterator applies in an identical manner to boost::detail::multi_array::array_iterator, as brought up by Thomas Klimpel multiple times now, most recently here: http://lists.boost.org/Archives/boost/2010/05/167125.php So whatever solution is deemed acceptable for bit_aligned_pixel_iterator should be applied to array_iterator as well, if possible.
Hi Adam, On Mon, May 31, 2010 at 1:25 PM, Adam Merz <adammerz@hotmail.com> wrote:
Hi Christian, this is ildjarn from the MSDN forums. :-]
Interesting disguise. ;-)
To be clear, it's not that MSVC10 doesn't understand that bit_aligned_pixel_iterator is a random access iterator, it's that bit_aligned_pixel_iterator *isn't* a random access iterator. I.e., std::iterator_traits< bit_aligned_pixel_iterator >::iterator_category is std::input_iterator_tag on every compiler/platform. It's just that MSVC10 refuses to instantiate std::copy given a destination iterator categorized as an input iterator (which is a good thing). If GIL had a regression test with a static assertion that boost::is_same< std::iterator_traits< bit_aligned_pixel_iterator<...> >::iterator_category, std::random_access_iterator_tag >::value == true, this would have been caught long ago. (hint, hint)
One perspective is that the bug here is that bit_aligned_pixel_iterator is trying to pass itself off as a random access iterator in the first place, despite the fact that it returns a proxy object rather than an actual reference, violating the standard's requirements for forward iterators. However, since it appears to work properly on all tested toolsets, that could be deemed an acceptable flaw, in which case the proper fix is to pass iterator_facade a category of std::random_access_iterator_tag rather than boost::random_access_traversal_tag. Easy enough. :-]
Easy enough, correct. I did this change to my local boost for a couple of gil's iterators. I have less compiler errors now. Do older MSVC versions and other compiler define std::random_access_iterator_tag? If though, I could add this fix to boost, as is.
Another perspective is that the bug here is caused by iterator_facade silently decaying into an input iterator despite being told to function as a random access iterator. This is documented behavior, so it's not a bug in and of itself, but the fact that it does so *silently* is a questionable design issue given that it's negatively affected at least two released libraries so far. I'm not sure what mechanism could be used to make the decay 'noisy' rather than silent, but this is clearly a gotcha for users of iterator_facade. Also, the fact that it decays into an input iterator rather than an input/output iterator seems arbitrary and less than ideal. Why doesn't it decay into boost::detail::input_output_iterator_tag? At least then it could be used as a destination for std::copy.
I really have a hard understanding where and when the metafunction boost::detail::iterator_facade_default_category is actually reducing bit_aligned_pixel_iterator to an input iterator? When I do this: using namespace boost; using namespace gil; typedef bit_aligned_image1_type< 1, gray_layout_t >::type image_t; typedef image_t::view_t view_t; typedef view_t::reference pixel_t; typedef bit_aligned_pixel_iterator< pixel_t > it_t; typedef iterator_traits< it_t >::iterator_category it_cat_t; it_cat_t is still boost::detail::iterator_category_with_traversal<std::input_iterator_tag,boost::random_access_traversal_tag>. Though, where does the "decaying" happen? Regards, Christian
Christian Henning <chhenning <at> gmail.com> writes:
Easy enough, correct. I did this change to my local boost for a couple of gil's iterators. I have less compiler errors now. Do older MSVC versions and other compiler define std::random_access_iterator_tag? If though, I could add this fix to boost, as is.
Yes, std::random_access_iterator_tag is standard and portable; you just need to #include <iterator> to use it. That said, I'm dubious as to whether this is the only fix (iterator_facade's category decaying mechanisms strike me as questionable) or even the correct fix (it violates the standard's requirements), so in my opinion this warrants a larger discussion, especially since at least one other released library is affected by the exact same issue.
I really have a hard understanding where and when the metafunction boost::detail::iterator_facade_default_category is actually reducing bit_aligned_pixel_iterator to an input iterator?
The instantiation path from bit_aligned_pixel_iterator to iterator_facade_default_category is: boost::gil::bit_aligned_pixel_iterator -> boost::iterator_facade -> boost::detail::iterator_facade_types -> boost::detail::facade_iterator_category -> boost::detail::facade_iterator_category_impl -> boost::detail::iterator_facade_default_category
When I do this: <snip code>
When bit_aligned_pixel_iterator.hpp lines 47 and 53 are both changed, this code produces it_cat_t == std::random_access_iterator_tag for me. Did you change both lines?
Though, where does the "decaying" happen?
The metaprogramming logic inside of iterator_facade_default_category is fairly straightforward, so I'm not sure what to say other than facade_iterator_category.hpp line 72. :-P
At Mon, 31 May 2010 23:25:44 +0000 (UTC), Adam Merz wrote:
To be clear, it's not that MSVC10 doesn't understand that bit_aligned_pixel_iterator is a random access iterator, it's that bit_aligned_pixel_iterator *isn't* a random access iterator. I.e., std::iterator_traits< bit_aligned_pixel_iterator >::iterator_category is std::input_iterator_tag on every compiler/platform.
The proxy reference alone is enough to make it not-a-random-access-iterator.
It's just that MSVC10 refuses to instantiate std::copy given a destination iterator categorized as an input iterator (which is a good thing). If GIL had a regression test with a static assertion that boost::is_same< std::iterator_traits< bit_aligned_pixel_iterator<...> >::iterator_category, std::random_access_iterator_tag >::value == true, this would have been caught long ago. (hint, hint)
Well, this iterator could also legitimately be classified as an output_iterator. The author of the iterator could multiply-inherit the iterator's tag from std::output_iterator_tag
One perspective is that the bug here is that bit_aligned_pixel_iterator is trying to pass itself off as a random access iterator in the first place, despite the fact that it returns a proxy object rather than an actual reference, violating the standard's requirements for forward iterators. However, since it appears to work properly on all tested toolsets, that could be deemed an acceptable flaw, in which case the proper fix is to pass iterator_facade a category of std::random_access_iterator_tag rather than boost::random_access_traversal_tag. Easy enough. :-]
That would be the “proper” way to do it (if we ignore the fact that you're lying to the standard library).
Another perspective is that the bug here is caused by iterator_facade silently decaying
It's not “silently decaying” into anything. It's following the rules.
into an input iterator despite being told to function as a random access iterator.
The iterator has been told to allow random access traversal, not to pass itself off as something it isn't. If you want to tell it the latter, just force it to use random_access_iterator_tag
This is documented behavior, so it's not a bug in and of itself, but the fact that it does so *silently*
What alternative would you suggest?
is a questionable design issue given that it's negatively affected at least two released libraries so far. I'm not sure what mechanism could be used to make the decay 'noisy' rather than silent, but this is clearly a gotcha for users of iterator_facade.
If there's a gotcha there, it should be handled with documentation.
Also, the fact that it decays into an input iterator rather than an input/output iterator seems arbitrary and less than ideal. Why doesn't it decay into boost::detail::input_output_iterator_tag? At least then it could be used as a destination for std::copy.
There's no decay. You simply haven't done anything to tell iterator_facade that the iterator is writable. Derive a tag from all the appropriate parts and it will do “what you want.”
Keep in mind that the issue surrounding bit_aligned_pixel_iterator applies in an identical manner to boost::detail::multi_array::array_iterator, as brought up by Thomas Klimpel multiple times now, most recently here: http://lists.boost.org/Archives/boost/2010/05/167125.php So whatever solution is deemed acceptable for bit_aligned_pixel_iterator should be applied to array_iterator as well, if possible.
FWIW, I advocate lying to the standard library here. The “new iterator categories” were somewhat mis-designed and will probably be deprecated sometime in the future anyway. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
participants (4)
-
Adam Merz -
Christian Henning -
David Abrahams -
Lubomir Bourdev