Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adopt LWG-3546 common_iterator's postfix-proxy is not quite right #1991

Merged
merged 11 commits into from
Oct 9, 2021
Merged

Adopt LWG-3546 common_iterator's postfix-proxy is not quite right #1991

merged 11 commits into from
Oct 9, 2021

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jun 9, 2021

Currently we are lacking the testing infrastructure for move only iterators.

Should we add it?

@miscco
Adopt LWG-3546
170101d 
We currently have no real infrastructure to test move only iterators, we should discuss that

Addresses microsoft#1965
@miscco miscco requested a review from a team as a code owner June 9, 2021 10:37
@StephanTLavavej StephanTLavavej added the LWG Library Working Group issue label Jun 10, 2021
@StephanTLavavej StephanTLavavej changed the title Adopt LWG-3546 Adopt LWG-3546 common_iterator's postfix-proxy is not quite right Jun 10, 2021
@CaseyCarter CaseyCarter mentioned this pull request Jun 10, 2021
Closed
36 tasks
timsong-cpp
timsong-cpp reviewed Jun 10, 2021
View reviewed changes
Copy link
Contributor

@timsong-cpp timsong-cpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is a patch for the common_iterator part of WG21-P2259, and I'm not sure how much sense it makes to implement just it without the other common_iterator changes.

stl/inc/iterator Outdated Show resolved Hide resolved
stl/inc/iterator Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Jun 11, 2021

This issue is a patch for the common_iterator part of WG21-P2259, and I'm not sure how much sense it makes to implement just it without the other common_iterator changes.

I made a mental note to investigate because I was baffled how we could have missed that proxy case. As is obvious now it should have been a written note ;)

I agree that we should merge this with the P2259 changes

@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Jun 30, 2021
@miscco
Copy link
Contributor Author

miscco commented Jul 10, 2021

I plan to have a fresh look at this next week

@miscco
Copy link
Contributor Author

miscco commented Jul 14, 2021

@CaseyCarter I believe this should be good for another round of review.

I did not add test coverage for the exotic cases though

CaseyCarter
CaseyCarter requested changes Jul 21, 2021
View reviewed changes
Copy link
Member

@CaseyCarter CaseyCarter left a comment •
edited by StephanTLavavej
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stuff needs fixing here; it may make more sense to implement this in the P2259 PR (#2059) or wait until that PR has been completed to finish this.

stl/inc/iterator Outdated Show resolved Hide resolved
stl/inc/iterator Outdated Show resolved Hide resolved
stl/inc/iterator Outdated Show resolved Hide resolved
@StephanTLavavej

This comment has been minimized.

Sign in to view
@cpplearner cpplearner mentioned this pull request Aug 13, 2021
Merged
@miscco
Copy link
Contributor Author

miscco commented Aug 24, 2021

@CaseyCarter I believe I addressed your comments, so this should be good for another round of review

cpplearner
cpplearner reviewed Sep 1, 2021
View reviewed changes
stl/inc/iterator Outdated Show resolved Hide resolved
@miscco @cpplearner
Update stl/inc/iterator
d125a26 
Co-authored-by: S. B. Tam <cpplearner@outlook.com>
@CaseyCarter CaseyCarter self-requested a review September 10, 2021 23:44
@CaseyCarter
Minor refactoring
2980ced 
* Move all the conditions for using the postfix-proxy into the helper concept for maximal short-circuiting benefit.
* Notice that the postfix-proxy reads from the iterator without requiring it to be `indirectly_readable`, add constraint and submit LWG issue to fix.
* Factor commonolity out of two proxy types into a common base
* drive-by fix the constraint on `operator->` by requiring `i.operator->()` to be valid for a `const _Iter` lvalue instead of and `_Iter` rvalue.
@CaseyCarter CaseyCarter removed their assignment Sep 24, 2021
@cpplearner cpplearner mentioned this pull request Oct 8, 2021
Closed
@CaseyCarter CaseyCarter self-assigned this Oct 8, 2021
@CaseyCarter
Copy link
Member

I'm mirroring this to an MSVC-internal PR. Please notify me if any further changes are pushed.

@CaseyCarter CaseyCarter merged commit 65a346d into microsoft:main Oct 9, 2021
@CaseyCarter
Copy link
Member

Thanks for helping to make common_iterator more conservative in its political beliefs.

@CaseyCarter CaseyCarter removed their assignment Oct 9, 2021
@miscco miscco deleted the LWG-3546-commoniterator-postfix branch October 9, 2021 05:26
AreaZR pushed a commit to AreaZR/STL that referenced this pull request Nov 4, 2021
@miscco @cpplearner
@CaseyCarter @AreaZR
Adopt LWG-3546 common_iterator's postfix-proxy is not quite right (…
5d3a3a8 
…microsoft#1991)

Addresses microsoft#1965

Co-authored-by: S. B. Tam <cpplearner@outlook.com>
Co-authored-by: Casey Carter <Casey@Carter.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timsong-cpp timsong-cpp timsong-cpp left review comments

@cpplearner cpplearner cpplearner left review comments

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Assignees
No one assigned
Labels
LWG Library Working Group issue ranges C++20/23 ranges
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@miscco @StephanTLavavej @CaseyCarter @cpplearner @timsong-cpp