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

Revert "make last work on any reversible collection (#42991), and fix it #43106

Merged
merged 2 commits into from
Nov 18, 2021
Merged

Revert "make last work on any reversible collection (#42991), and fix it #43106

merged 2 commits into from
Nov 18, 2021

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 16, 2021

@vtjnash
Revert "make last work on any reversible collection (#42991)"
57fa0ca 
This reverts commit 1f484c3 since it
seems to cause excess breakage for the benefits it brings.
@vtjnash
add back last for every type that defines Iterators.reverse
3049893 
Fixes #42943
Fixes #43101
Replaces #42991
JeffBezanson
JeffBezanson reviewed Nov 16, 2021
View reviewed changes
base/iterators.jl
@@ -882,6 +890,7 @@ function iterate(it::Cycle, state)
end

reverse(it::Cycle) = Cycle(reverse(it.xs))
last(it::Cycle) = last(it.xs)
Copy link
Member

Choose a reason for hiding this comment

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

This (and the method on the previous line!) doesn't seem right. The sequence doesn't converge.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't feel like I can delete them in this PR though

@MasonProtter MasonProtter mentioned this pull request Nov 17, 2021
Merged
@Sacha0
Copy link
Member

Sacha0 commented Nov 18, 2021

Is anything preventing merging this revert? It seems worth unbreaking consumers' code till an alternative (e.g. #43110 (comment)) lands? :)

@JeffBezanson
Copy link
Member

It seems to me if we have the fallback chain last -> reverse -> indexing, then everything works for indexable objects, plus last continues to work for objects that only have reverse. For example, a doubly-linked list that you can reverse iterate efficiently, but not index efficiently.

@vtjnash vtjnash merged commit 0070f82 into master Nov 18, 2021
@vtjnash vtjnash deleted the jn/revert-42991 branch November 18, 2021 21:03
@vtjnash
Copy link
Member Author

vtjnash commented Nov 18, 2021

Yes, but that was slightly more breaking still than necessary, and if you can decide to explicitly define the reverse iterator, then you can likely define this method easily also.

@thofma thofma mentioned this pull request Nov 19, 2021
Closed
@KristofferC KristofferC mentioned this pull request Dec 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeffBezanson JeffBezanson JeffBezanson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vtjnash @Sacha0 @JeffBezanson