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

Add iterator method .rfold(init, function); the reverse of fold #44682

Merged
merged 7 commits into from
Sep 22, 2017

Conversation

bluss
Copy link
Member

@bluss bluss commented Sep 18, 2017

rfold is the reverse version of fold.

Fold allows iterators to implement a different (non-resumable) internal
iteration when it is more efficient than the external iteration implemented
through the next method. (Common examples are VecDeque and .chain()).

Introduce rfold() so that the same customization is available for reverse
iteration. This is achieved by both adding the method, and by having the
Rev<I> adaptor connect Rev::rfold → I::fold and Rev::fold → I::rfold.

On the surface, rfold(..) is just .rev().fold(..), but the special case
implementations allow a data structure specific fold to be used through for
example .iter().rev(); we thus have gains even for users never calling exactly
rfold themselves.

Adaptors are things that take iterators and adapt them into other
iterators. With this definition, fold is just a usual method, because it
doesn't normally make an iterator.
rfold is the reverse version of fold.

Fold allows iterators to implement a different (non-resumable) internal
iteration when it is more efficient than the external iteration
implemented through the next method. (Common examples are VecDeque and
.chain()).

Introduce rfold() so that the same customization is available for
reverse iteration. This is achieved by both adding the method, and by
having the Rev<I> adaptor connect Rev::rfold -> I::fold, Rev::fold -> I::rfold.
With both in place, we can cross them over in rev, and we give rfold
behaviour to .rev().fold() and so on.
@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@bluss bluss changed the title Add iterator method .rfold(accumulator, function); the reverse of fold Add iterator method .rfold(init, function); the reverse of fold Sep 18, 2017
@aturon
Copy link
Member

aturon commented Sep 18, 2017

cc @rust-lang/libs

@cuviper
Copy link
Member

cuviper commented Sep 18, 2017

Note that I added FlatMap::fold in #44577 -- it probably deserves rfold too.

@dtolnay
Copy link
Member

dtolnay commented Sep 19, 2017

What determines whether this goes on the DoubleEndedIterator trait like rfind or on the Iterator trait like rposition?

fn rfold<B, F>(self, accum: B, f: F) -> B
where
    F: FnMut(B, Self::Item) -> B,
    Self: Sized + DoubleEndedIterator;

@dtolnay
Copy link
Member

dtolnay commented Sep 19, 2017

Ah, we already had that discussion in #39399 and #39480.

I would be on board with this addition.

@alexcrichton
Copy link
Member

Seems like a fine addition to me!

@sfackler
Copy link
Member

SGTM

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

@bluss r=me but please file a tracking issue and include the issue number in the #[unstable].

@bluss
Copy link
Member Author

bluss commented Sep 19, 2017

great, done.

@bors r=dtolnay

@bors
Copy link
Contributor

bors commented Sep 19, 2017

📌 Commit 41a4226 has been approved by dtolnay

@arielb1 arielb1 added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 19, 2017
@bors
Copy link
Contributor

bors commented Sep 21, 2017

⌛ Testing commit 41a4226 with merge 23a84ab...

bors added a commit that referenced this pull request Sep 21, 2017
Add iterator method .rfold(init, function); the reverse of fold

rfold is the reverse version of fold.

Fold allows iterators to implement a different (non-resumable) internal
iteration when it is more efficient than the external iteration implemented
through the next method. (Common examples are VecDeque and .chain()).

Introduce rfold() so that the same customization is available for reverse
iteration. This is achieved by both adding the method, and by having the
Rev\<I> adaptor connect Rev::rfold → I::fold and Rev::fold → I::rfold.

On the surface, rfold(..) is just .rev().fold(..), but the special case
implementations allow a data structure specific fold to be used through for
example .iter().rev(); we thus have gains even for users never calling exactly
rfold themselves.
@bors
Copy link
Contributor

bors commented Sep 21, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

@bors: retry

Looks like this was spuriously canceled? not sure why...

@bors
Copy link
Contributor

bors commented Sep 21, 2017

⌛ Testing commit 41a4226 with merge 17600c1...

bors added a commit that referenced this pull request Sep 21, 2017
Add iterator method .rfold(init, function); the reverse of fold

rfold is the reverse version of fold.

Fold allows iterators to implement a different (non-resumable) internal
iteration when it is more efficient than the external iteration implemented
through the next method. (Common examples are VecDeque and .chain()).

Introduce rfold() so that the same customization is available for reverse
iteration. This is achieved by both adding the method, and by having the
Rev\<I> adaptor connect Rev::rfold → I::fold and Rev::fold → I::rfold.

On the surface, rfold(..) is just .rev().fold(..), but the special case
implementations allow a data structure specific fold to be used through for
example .iter().rev(); we thus have gains even for users never calling exactly
rfold themselves.
@bors
Copy link
Contributor

bors commented Sep 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: dtolnay
Pushing 17600c1 to master...

@bluss bluss deleted the iter-rfold branch November 10, 2017 19:02
bors added a commit that referenced this pull request Nov 17, 2017
Short-circuiting internal iteration with Iterator::try_fold & try_rfold

These are the core methods in terms of which the other methods (`fold`, `all`, `any`, `find`, `position`, `nth`, ...) can be implemented, allowing Iterator implementors to get the full goodness of internal iteration by only overriding one method (per direction).

Based off the `Try` trait, so works with both `Result` and `Option` (:tada: #42526).  The `try_fold` rustdoc examples use `Option` and the `try_rfold` ones use `Result`.

AKA continuing in the vein of PRs #44682 & #44856 for more of `Iterator`.

New bench following the pattern from the latter of those:
```
test iter::bench_take_while_chain_ref_sum          ... bench:   1,130,843 ns/iter (+/- 25,110)
test iter::bench_take_while_chain_sum              ... bench:     362,530 ns/iter (+/- 391)
```

I also ran the benches without the `fold` & `rfold` overrides to test their new default impls, with basically no change.  I left them there, though, to take advantage of existing overrides and because `AlwaysOk` has some sub-optimality due to #43278 (which 45225 should fix).

If you're wondering why there are three type parameters, see issue #45462

Thanks for @bluss for the [original IRLO thread](https://internals.rust-lang.org/t/pre-rfc-fold-ok-is-composable-internal-iteration/4434) and the rfold PR and to @cuviper for adding so many folds, [encouraging me](#45379 (comment)) to make this PR, and finding a catastrophic bug in a pre-review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants