Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#5697 closed Bugs (fixed)

iterator_facade::operator-> is broken for proxy references

Reported by: Jeffrey Hellrung <jeffrey.hellrung@…> Owned by: Dave Abrahams
Milestone: To Be Determined Component: iterator
Version: Boost 1.47.0 Severity: Problem
Keywords: iterator_facade, operator->, operator_arrow_proxy Cc:

Description

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

Attachments (4)

iterator_facade.patch.hpp (4.0 KB) - added by Jeffrey Hellrung <jeffrey.hellrung@…> 6 years ago.
iterator_facade.patch.2.hpp (4.0 KB) - added by Jeffrey Hellrung <jeffrey.hellrung@…> 6 years ago.
iterator_facade.hpp.patch (4.4 KB) - added by Jeffrey Hellrung <jeffrey.hellrung@…> 6 years ago.
iterator_facade.cpp.patch (3.0 KB) - added by Jeffrey Hellrung <jeffrey.hellrung@…> 6 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by Dave Abrahams

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

Thanks!

Changed 6 years ago by Jeffrey Hellrung <jeffrey.hellrung@…>

Attachment: iterator_facade.patch.hpp added

comment:2 Changed 6 years ago by 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).

comment:3 Changed 6 years ago by Jeffrey Hellrung <jeffrey.hellrung@…>

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

comment:4 Changed 6 years ago by Dave Abrahams

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

Changed 6 years ago by Jeffrey Hellrung <jeffrey.hellrung@…>

Attachment: iterator_facade.patch.2.hpp added

comment:5 Changed 6 years ago by 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!).

comment:6 Changed 6 years ago by Dave Abrahams

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

comment:7 Changed 6 years ago by 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.

comment:8 Changed 6 years ago by 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?

Changed 6 years ago by Jeffrey Hellrung <jeffrey.hellrung@…>

Attachment: iterator_facade.hpp.patch added

comment:9 Changed 6 years ago by 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.

Changed 6 years ago by Jeffrey Hellrung <jeffrey.hellrung@…>

Attachment: iterator_facade.cpp.patch added

comment:10 Changed 6 years ago by Jeffrey Hellrung <jeffrey.hellrung@…>

:: ping ::

How likely will this sneak into 1.48?

comment:11 Changed 6 years ago by anonymous

Jeff, is this earlier thread describing the same problem?

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

comment:12 in reply to:  11 Changed 6 years ago by 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.

comment:13 Changed 6 years ago by 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?

comment:14 Changed 6 years ago by 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.

comment:15 Changed 6 years ago by 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.

comment:16 Changed 6 years ago by 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.

comment:17 Changed 6 years ago by Jeremiah Willcock

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

comment:18 Changed 6 years ago by Jeremiah Willcock

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

comment:19 Changed 6 years ago by 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.

comment:20 Changed 6 years ago by jeffrey.hellrung

Resolution: fixed
Status: newclosed

comment:21 Changed 6 years ago by jeffrey.hellrung

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Dave Abrahams.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.