Skip to content

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
@miscco
Copy link
Contributor Author

miscco commented Jul 31, 2020

Given that the probability of using counted_iterator internally I have moved it to <iterator> where it belongs.

Thakns to @AdamBucior

… coverage

Giveup on `counted_iterator` ever working in MSVC's permissive mode; add strict_concepts test matrix and change winsdk_concepts to strict_winsdk_concepts.

Also communicate the constant iterator type to which a `test::iterator` can be converted via an oddly-named nested type alias `Consterator` that shouldn't inadvertently match any other use cases.

Wiggle converting constructor tests a bit to ensure they're testing `counted_iterator`'s conversions and not the adapted iterator's conversions.

Guard cross-type difference and comparison tests with their prerequisite condition `common_with` which doesn't hold for move-only iterators.

Give `_Same_sequence` a descriptive comment.
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.

Yes, I'm requesting changes to the things that I just changed. I can totally do that.

@CaseyCarter CaseyCarter removed their assignment Aug 11, 2020
@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

CaseyCarter and others added 2 commits August 13, 2020 18:44
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