Skip to content

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

Merged
CaseyCarter merged 11 commits intomicrosoft:mainfrom
miscco:LWG-3546-commoniterator-postfix
Oct 9, 2021
Merged

Adopt LWG-3546 common_iterator's postfix-proxy is not quite right#1991
CaseyCarter merged 11 commits intomicrosoft:mainfrom
miscco:LWG-3546-commoniterator-postfix

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.

@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 suggested changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

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

@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
@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
Merge remote-tracking branch 'origin/main' into LWG-3546-commoniterat…
d72bf4c 
…or-postfix
@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
Contributor

CaseyCarter commented Oct 8, 2021

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
Contributor

CaseyCarter commented Oct 9, 2021

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
SiliconA-Z pushed a commit to SiliconA-Z/STL that referenced this pull request Nov 4, 2021
@miscco @cpplearner
@CaseyCarter @SiliconA-Z
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

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+3 more reviewers

@cpplearner cpplearner cpplearner left review comments

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

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

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