Modify

Ticket #5697 (closed Bugs: fixed)

Opened 3 years ago

Last modified 2 years ago

iterator_facade::operator-> is broken for proxy references

Reported by: Jeffrey Hellrung <jeffrey.hellrung@…> Owned by: dave
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

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

Change History

comment:1 Changed 3 years ago by dave

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

Thanks!

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

comment:2 Changed 3 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 3 years ago by Jeffrey Hellrung <jeffrey.hellrung@…>

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

comment:4 Changed 3 years ago by dave

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

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

comment:5 Changed 3 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 3 years ago by dave

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

comment:7 Changed 3 years ago by dave

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 3 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 3 years ago by Jeffrey Hellrung <jeffrey.hellrung@…>

comment:9 Changed 3 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 3 years ago by Jeffrey Hellrung <jeffrey.hellrung@…>

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

:: ping ::

How likely will this sneak into 1.48?

comment:11 follow-up: ↓ 12 Changed 2 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 2 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 2 years ago by jewillco

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 2 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 2 years ago by jewillco

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 2 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 2 years ago by jewillco

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

comment:18 Changed 2 years ago by jewillco

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

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

  • Status changed from new to closed
  • Resolution set to fixed

comment:21 Changed 2 years ago by jeffrey.hellrung

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

View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
Author


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

 
Note: See TracTickets for help on using tickets.