Skip to content

Correct iterator adaptor Chain #27991

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

Merged
merged 1 commit into from
Aug 26, 2015
Merged

Conversation

bluss
Copy link
Member

@bluss bluss commented Aug 25, 2015

Correct iterator adaptor Chain

The iterator protocol specifies that the iteration ends with the return
value None from .next() (or .next_back()) and it is unspecified
what further calls return. The chain adaptor must account for this in
its DoubleEndedIterator implementation.

It uses three states:

  • Both a and b are valid
  • Only the Front iterator (a) is valid
  • Only the Back iterator (b) is valid

The fourth state (neither iterator is valid) only occurs after Chain has
returned None once, so we don't need to store this state.

Fixes #26316

@rust-highfive
Copy link
Contributor

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@bluss
Copy link
Member Author

bluss commented Aug 25, 2015

r? @gankro

@rust-highfive rust-highfive assigned Gankra and unassigned nikomatsakis Aug 25, 2015
@bluss bluss force-pushed the chain-rev branch 4 times, most recently from 90e9de5 to a5d6eaa Compare August 25, 2015 12:31
@alexcrichton
Copy link
Member

Could you add a comment to ChainState indicating why it exists? (basically the description of this PR). Other than that, though, looks good to me, thanks!

The iterator protocol specifies that the iteration ends with the return
value `None` from `.next()` (or `.next_back()`) and it is unspecified
what further calls return. The chain adaptor must account for this in
its DoubleEndedIterator implementation.

It uses three states:

- Both `a` and `b` are valid
- Only the Front iterator (`a`) is valid
- Only the Back iterator (`b`) is valid

The fourth state (neither iterator is valid) only occurs after Chain has
returned None once, so we don't need to store this state.

Fixes rust-lang#26316
@bluss
Copy link
Member Author

bluss commented Aug 25, 2015

Updated with a comment.

If only one end of the iterator is used and everything is inlined, llvm should see that the state field only takes on two values (For example Both and Back), so it could behave like it did before.

@alexcrichton
Copy link
Member

@bors: r+ 35eb3e8

Thanks!

bors added a commit that referenced this pull request Aug 26, 2015
Correct iterator adaptor Chain

The iterator protocol specifies that the iteration ends with the return
value `None` from `.next()` (or `.next_back()`) and it is unspecified
what further calls return. The chain adaptor must account for this in
its DoubleEndedIterator implementation.

It uses three states:

- Both `a` and `b` are valid
- Only the Front iterator (`a`) is valid
- Only the Back iterator (`b`) is valid

The fourth state (neither iterator is valid) only occurs after Chain has
returned None once, so we don't need to store this state.

Fixes #26316
@bors
Copy link
Collaborator

bors commented Aug 26, 2015

⌛ Testing commit 35eb3e8 with merge ef3255b...

@alexcrichton
Copy link
Member

Looks like one of the windows builders disappeared, but this passed tests everywhere (including windows) so merging manually.

@alexcrichton alexcrichton merged commit 35eb3e8 into rust-lang:master Aug 26, 2015
@bluss bluss deleted the chain-rev branch August 26, 2015 16:11
@bluss
Copy link
Member Author

bluss commented Aug 26, 2015

Thanks for the adjustment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants