iterator_facade::operator-> is broken for proxy references

[Jeffrey Hellrung <jeffrey.hellrung@…>]

For the detailed discussion:

​http://comments.gmane.org/gmane.comp.lib.boost.devel/221041

Briefly, when using a proxy reference in one's iterator derived from iterator_facade, the implementation of operator-> tries to construct a value_type const * from the expression &x, where x has (declared) type reference. I see no reason why a reference* (a pointer to a proxy reference object) could be expected to be a convertible to a value_type const *. I believe a correct implementation of operator_arrow_proxy would store the proxy reference object, not an object of the value_type. As far as I know, this does not violate any iterator requirements.

I will gladly submit a patch once it is acknowledged that this is, indeed, a bug (or before, if requested).

[Dave Abrahams]

Please do submit a patch; that will make the report much easier to evaluate.

Thanks!

[Jeffrey Hellrung <jeffrey.hellrung@…>]

I'm a novice at creating patches; hopefully iterator_facade.patch.hpp is sufficient, and if not, let me know. I changed iterator_facade as indicated in the patch and, at least, everything built correctly. If you want, I can try figuring out how to run the Boost.Iterator tests and verify that they all pass (or not).

[Jeffrey Hellrung <jeffrey.hellrung@…>]

"built correctly" == "without compiler errors" :/

[Dave Abrahams]

please run bjam in libs/iterator/test under BOOST_ROOT and see if any tests fail.

[Jeffrey Hellrung <jeffrey.hellrung@…>]

Okay, I ran the iterator tests, and iterator_facade.cpp failed to compile at line 107, "p->mutator();". To get that line to compile (and the test to pass), I removed some consts from the operator_arrow_dispatch::proxy, as shown in the second attachment (iterator_facade.patch.2.hpp). Looks like all iterator tests pass now, but I'm not really sure if the consts would be necessary on other compilers (I'm using MSVC9 on Windows 7). Let me know what you think when you get a chance (and thanks for looking at this with me!).

[Dave Abrahams]

(minor detail: next time make the attachments end with .patch instead of .hpp and they will render as patches.)

[Dave Abrahams]

Your patch looks excellent. I don't think there's any reason to use implicit_cast in there, though. At this point, it should just be

return boost::addressof(x);

If you can fix that, and add a test case that fails without your patch and passes otherwise, I'll be happy to commit this.

[anonymous]

Agreed on both accounts; I was just trying to maintain the same style as before.

I will change the patch accordingly (with a .patch extension) and add a test case to...iterator_facade.cpp, I suppose?

[Jeffrey Hellrung <jeffrey.hellrung@…>]

I have uploaded a patch for iterator_facade.hpp, but I'm having issues uploading a patch for iterator_facade.cpp (from libs/iterator/test). The spam filter keeps thinking I'm uploading spam :( I guess I will email the attachment directly to you, Dave, and if it gets lost in the shuffle, I can try uploading again...

The test fails for the present version of iterator_facade with

compile-c-c++ ..\..\..\bin.v2\libs\iterator\test\iterator_facade.test\msvc-9.0\debug\threading-multi\iterator_facade.obj iterator_facade.cpp D:\boost_1_47_0\boost/iterator/iterator_facade.hpp(327) : error C2664: 'boost::implicit_cast' : cannot convert parameter 1 from 'wrapper<T> *' to 'boost::detail::operator_arrow_proxy<T>'

with [

T=int &

] and [

T=wrapper<int>

] No constructor could take the source type, or constructor overload resolution was ambiguous

...as expected.

[Jeffrey Hellrung <jeffrey.hellrung@…>]

:: ping ::

How likely will this sneak into 1.48?

[anonymous]

Jeff, is this earlier thread describing the same problem?

​http://thread.gmane.org/gmane.comp.lib.boost.devel/198562

[Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung@…>]

Replying to anonymous:

Jeff, is this earlier thread describing the same problem?

​http://thread.gmane.org/gmane.comp.lib.boost.devel/198562

I believe so.

[Jeremiah Willcock]

I think that bug #4313 is reporting the same issue as well, and also has a patch. Should I close that one as a duplicate now?

[Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung@…>]

Looks like it, so yes, you should probably close #4313 as duplicate. Feel free to apply the attached patches, although I'm hoping to have some free time in the coming months to actually learn the development process and get this and other Iterator tickets taken care of.

[Jeremiah Willcock]

Which of the patches would you like me to apply? One of them from this ticket or the one from #4313? I'll go ahead and close #4313 anyway.

[Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung@…>]

If willing, please apply iterator_facade.*pp.patch from this ticket (the latter 2 patches). I believe they are more complete than the patch provided in #4313.

[Jeremiah Willcock]

(In [77723]) Applied patches from #5697; refs #5697

[Jeremiah Willcock]

I've applied the patches you wanted -- please close the bug if you think it works now.

[Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung@…>]

Awesome, thanks. Once I reset my SVN password (::sigh::) I'll review the test result matrix and close this ticket.

[jeffrey.hellrung]

(In [78184]) merging from trunk; fix #5127 from M. Morin; fix for refs #5697