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.

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

Implement counted_iterator #1083

Merged
merged 42 commits into from
Aug 14, 2020
Merged

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jul 24, 2020

This is a work in progress implementation of std::counted_iterator

I am quite happy where I am right now. However, there are quite some sharp edges I would like to discuss.

  • The standard has a frequent precondition that both iterators should be of the same range. It is rather tough to implement that. I tried my best but maybe you have a better idea.

  • I have strengthened noexcept where I feel comfortable. There are most certainly other possibilities

  • There is something strange with the iter_move requirement, as it fails for std::vector<int> in the death tests.

I have some further work in progress regarding actual unit tests beyond the iterator machinery, but they need way more polish so I have not yet added them.

Partially addresses #39

@StephanTLavavej StephanTLavavej added cxx20 C++20 feature ranges C++20/23 ranges labels Jul 24, 2020
@miscco miscco force-pushed the counted_iterator branch from 64a8b80 to 3c91053 Compare July 24, 2020 12:48
@miscco
Copy link
Contributor Author

miscco commented Jul 24, 2020

I have added some unit tests. There are two remaining major problems:

  1. For whatever reason the constraint requires input_iterator<_Iter> breaks for iter_move.
  2. My Verify_range function was broken

I have uncommented them for now to get the tests passing -at least locally- so that we can flesh out some other potential bugs through other compiler/configurations

NOTE: this is also missing tests for the iter_move and iter_swap as I am not yet sure where to put them. I am leaning towards ranges_iterator_machinery

@miscco miscco marked this pull request as ready for review July 24, 2020 21:06
@miscco miscco requested a review from a team as a code owner July 24, 2020 21:06
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 a quick sweep over the class itself.

Copy link
Contributor Author

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Applied the review comments as far as I can. there were some where I had a question

@CaseyCarter CaseyCarter mentioned this pull request Jul 27, 2020
@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

Commit STL's tweaks.

Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
Comment change I overlooked in the previous update.

Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
@CaseyCarter CaseyCarter self-assigned this Aug 14, 2020
@CaseyCarter CaseyCarter merged commit 1e42166 into microsoft:master Aug 14, 2020
@CaseyCarter
Copy link
Contributor

I knew I could count on you to get this implemented! 😎

@CaseyCarter CaseyCarter removed their assignment Aug 14, 2020
@miscco miscco deleted the counted_iterator branch August 15, 2020 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature ranges C++20/23 ranges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants