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

Implement common_iterator #1092

Merged
merged 44 commits into from
Sep 4, 2020
Merged

Implement common_iterator #1092

merged 44 commits into from
Sep 4, 2020

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jul 26, 2020

This implements common_iterator

I am at a point where I would like to get some feedback regarding certain issues

  1. I have implemented it via a union rather than pulling in variant. unions scare me.
  2. I had to additionally define the "classic" copy constructor/assignment because of the union. Do we also need move assignment?
  3. I have never really worked with unions ( unions scare me) so I freely stole from optional/variant That is the reason I moved _Construct_in_place and _Nontrivial_dummy to xutility. Hiss if it does not belong there.
  4. I am slightly concerned what happens if some of the preconditions are not met in release. unions scare me.
  5. Similarly to counted_iterator i get strange errors about requires input_iterator<_Iter>
  6. Tests. Urgh. operator->() is especially terrible thoughts?

@miscco miscco requested a review from a team as a code owner July 26, 2020 20:00
miscco
miscco commented Jul 26, 2020
View reviewed changes
stl/inc/xutility Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added cxx20 C++20 feature ranges C++20/23 ranges labels Jul 26, 2020
@miscco miscco force-pushed the common_iterator branch from de0411b to d0787c2 Compare July 27, 2020 10:00
@miscco
Copy link
Contributor Author

miscco commented Jul 27, 2020

@CaseyCarter the test currently fail because in the test machinery we require input_iterator for iter_swap.

So the question is is that requirement valid or should we expand it to indirectly_swappable

@miscco
Copy link
Contributor Author

miscco commented Jul 27, 2020 •
edited
Loading

So I build this throwing_iterator to create valueless common_iterator. However it seems that I cannot use them as is in the death tests any exception directly terminates. Any suggestion on how to verify the asserts regarding the valueless_by_exception case?

@CaseyCarter CaseyCarter mentioned this pull request Jul 27, 2020
Closed
@miscco
Copy link
Contributor Author

miscco commented Jul 28, 2020

I might be just spectacularly stupid. It seems I tried to swap elements of a const range and wondered why it didnt work...

At least locally all tests now work. Lets see what clang brings

@miscco miscco force-pushed the common_iterator branch from 588bd2e to 9d9a8ff Compare July 28, 2020 15:00
@miscco
Copy link
Contributor Author

miscco commented Jul 28, 2020

I believe these test failures require maintainer support

@miscco
Copy link
Contributor Author

miscco commented Jul 29, 2020

So I was thinking a bit about exception safety and came up with the horror that is counted_iterator::operator=

Is that overkill?

@miscco
Copy link
Contributor Author

miscco commented Aug 31, 2020

@CaseyCarter This should be ready for the next round of reviews

@CaseyCarter CaseyCarter self-requested a review September 2, 2020 21:31
CaseyCarter
CaseyCarter reviewed Sep 3, 2020
View reviewed changes
tests/std/tests/P0896R4_common_iterator/env.lst Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator/test.cpp Outdated Show resolved Hide resolved
stl/inc/iterator Outdated Show resolved Hide resolved
stl/inc/iterator Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator/test.cpp Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator/test.cpp Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator_death/test.cpp Show resolved Hide resolved
CaseyCarter
CaseyCarter suggested changes Sep 3, 2020
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Just some tweaky things which I'll fixup right now.

tests/std/tests/P0896R4_common_iterator_death/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator_death/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator_death/test.cpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter removed their assignment Sep 3, 2020
StephanTLavavej
StephanTLavavej requested changes Sep 3, 2020
View reviewed changes
tests/std/tests/winsdk_concepts_matrix.lst Outdated Show resolved Hide resolved
stl/inc/iterator Outdated Show resolved Hide resolved
stl/inc/iterator Show resolved Hide resolved
stl/inc/iterator Outdated Show resolved Hide resolved
stl/inc/iterator Outdated Show resolved Hide resolved
stl/inc/iterator Show resolved Hide resolved
stl/inc/iterator Show resolved Hide resolved
stl/inc/iterator Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator/test.cpp Show resolved Hide resolved
tests/std/tests/P0896R4_common_iterator/test.cpp Show resolved Hide resolved
@CaseyCarter CaseyCarter self-assigned this Sep 4, 2020
@CaseyCarter CaseyCarter merged commit 30e7a1b into microsoft:master Sep 4, 2020
@CaseyCarter
Copy link
Contributor

Thanks for implementing this decidedly un-common Ranges component! 😎

@miscco miscco deleted the common_iterator branch September 4, 2020 18:15
@CaseyCarter CaseyCarter removed their assignment Sep 4, 2020
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Sep 4, 2020
@CaseyCarter
Revert "Implement common_iterator (microsoft#1092)"
3a7a615 
This reverts commit 30e7a1b.
CaseyCarter added a commit that referenced this pull request Sep 4, 2020
@CaseyCarter
Revert "Implement common_iterator (#1092)" (#1265)
6156ec3 
This reverts commit 30e7a1b.
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Sep 5, 2020
@CaseyCarter
Revert "Revert "Implement common_iterator (microsoft#1092)" (microsof…
6d52216 
…t#1265)"

This reverts commit 6156ec3.
CaseyCarter added a commit that referenced this pull request Sep 16, 2020
@CaseyCarter
Revert "Revert "Implement common_iterator (#1092)" (#1265)" (#1268)
0e4f6b5 
This reverts commit 6156ec3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@statementreply statementreply statementreply left review comments

@cpplearner cpplearner cpplearner left review comments

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees
No one assigned
Labels
cxx20 C++20 feature ranges C++20/23 ranges
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@miscco @statementreply @AdamBucior @CaseyCarter @StephanTLavavej @cpplearner