Change advance(_back)_by to return the remainder instead of the number of processed elements #92284

the8472 · 2021-12-26T03:16:17Z

When advance_by can't advance the iterator by the number of requested elements it now returns the amount by which it couldn't be advanced instead of the amount by which it did.

This simplifies adapters like chain, flatten or cycle because the remainder doesn't have to be calculated as the difference between requested steps and completed steps anymore.

Additionally switching from Result<(), usize> to Result<(), NonZeroUsize> reduces the size of the result and makes converting from/to a usize representing the number of remaining steps cheap.

rust-highfive · 2021-12-26T03:16:20Z

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

bors · 2022-01-11T18:07:53Z

☔ The latest upstream changes (presumably #92070) made this pull request unmergeable. Please resolve the merge conflicts.

scottmcm · 2022-02-26T17:15:22Z

This was made a Result because a stable advance_by would be handy as a non-lazy skip. And then .advance_by(3).unwrap() is nice to have available for the same kinds of places as you might use .next().unwrap().

That said, I can't find the discussion for why the number returned is the number consumed as opposed to the number unconsumed. Perhaps it could make sense to change it to -> Result<(), NonZeroUsize>? Then the machine code generated would be the same as with usize, but the type goodness would still be available. And the "if they didn't ask to skip anything then you return Ok(())" part is clear in the type signature.

the8472 · 2022-02-26T17:30:12Z

And then .advance_by(3).unwrap() is nice to have available for the same kinds of places as you might use .next().unwrap().

next().unwrap() gives you a value though. advance_by(3).unwrap() doesn't provide anything used and is usually followed by a next() which would then fail anyway.

scottmcm · 2022-02-26T17:33:29Z

next().unwrap() gives you a value though. advance_by(3).unwrap() doesn't provide anything used and is usually followed by a next() which would then fail anyway.

It's critical for correctness with unfused iterators, though.

it.advance_by(4); it.next() can actually return a value despite the fact that the advance_by hit the end of the iterator.

the8472 · 2022-02-26T17:45:01Z

But that's already handled by nth(), so you rarely have to implement it directly.

dtolnay · 2022-12-23T21:51:16Z

Reassigning yaahc's reviews as she has stepped down from the review rotation.

r? rust-lang/libs-api

scottmcm · 2022-12-23T22:03:46Z

But that's already handled by nth(), so you rarely have to implement it directly.

That's only if next is after it, and after it immediately.

It doesn't help for it.advance_by(1).ok()?; it.all(…), for example.

Dylan-DPC · 2023-02-22T14:18:06Z

@the8472 any updates on this pr?

the8472 · 2023-02-22T18:55:14Z

I'll give this a stab.

Perhaps it could make sense to change it to -> Result<(), NonZeroUsize>? Then the machine code generated would be the same as with usize, but the type goodness would still be available. And the "if they didn't ask to skip anything then you return Ok(())" part is clear in the type signature.

rustbot · 2023-03-13T19:09:40Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

library/core/src/iter/traits/double_ended.rs

library/core/src/ops/index_range.rs

scottmcm · 2023-03-13T19:32:16Z

From a quick scan this looks good. I'll try to do a proper review later today.

How much ACP-age would be expected for this? Am I allowed to just r+ it, or does it need a libs-api second somewhere?

the8472 · 2023-03-13T19:43:04Z

No idea. I find the Result + NonZeroUsize a bit wordy for what it does, so maybe that warrants some discussion. Then again, it's only chafing because I had to rewrite that dozens of times 😫

library/core/src/iter/traits/iterator.rs

scottmcm · 2023-03-21T18:41:12Z

library/core/tests/slice.rs

        assert_eq!(iter.as_slice(), &v[i..]);
    }

    let mut iter = v.iter();
-    assert_eq!(iter.advance_by(v.len() + 1), Err(v.len()));
+    assert_eq!(iter.advance_by(v.len() + 1),  Err(NonZeroUsize::new(1).unwrap()));


nit: looks like this got an extra space, which I'm surprised tidy is happyabout

Suggested change

assert_eq!(iter.advance_by(v.len() + 1), Err(NonZeroUsize::new(1).unwrap()));

assert_eq!(iter.advance_by(v.len() + 1), Err(NonZeroUsize::new(1).unwrap()));

library/alloc/src/collections/vec_deque/into_iter.rs

library/core/src/iter/adapters/chain.rs

library/core/src/iter/adapters/flatten.rs

library/core/src/iter/traits/iterator.rs

library/core/tests/iter/adapters/chain.rs

scottmcm · 2023-03-21T21:19:11Z

r=me with /~https://github.com/rust-lang/rust/pull/92284/files#r1143884909 addressed.

…ze>` A successful advance is now signalled by returning `0` and other values now represent the remaining number of steps that couldn't be advanced as opposed to the amount of steps that have been advanced during a partial advance_by. This simplifies adapters a bit, replacing some `match`/`if` with arithmetic. Whether this is beneficial overall depends on whether `advance_by` is mostly used as a building-block for other iterator methods and adapters or whether we also see uses by users where `Result` might be more useful.

the8472 · 2023-03-27T21:02:51Z

@bors r=scottmcm

bors · 2023-03-27T21:02:53Z

📌 Commit 4180793 has been approved by scottmcm

It is now in the queue for this repository.

Rollup of 8 pull requests Successful merges: - rust-lang#91793 (socket ancillary data implementation for FreeBSD (from 13 and above).) - rust-lang#92284 (Change advance(_back)_by to return the remainder instead of the number of processed elements) - rust-lang#102472 (stop special-casing `'static` in evaluation) - rust-lang#108480 (Use Rayon's TLV directly) - rust-lang#109321 (Erase impl regions when checking for impossible to eagerly monomorphize items) - rust-lang#109470 (Correctly substitute GAT's type used in `normalize_param_env` in `check_type_bounds`) - rust-lang#109562 (Update ar_archive_writer to 0.1.3) - rust-lang#109629 (remove obsolete `givens` from regionck) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned yaahc Dec 26, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 26, 2021

the8472 mentioned this pull request Dec 26, 2021

Tracking Issue for feature(iter_advance_by) #77404

Open

3 tasks

This comment has been minimized.

Sign in to view

the8472 force-pushed the simplify-advance-by branch from 27c381e to f2beb02 Compare December 26, 2021 12:49

the8472 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 26, 2021

the8472 mentioned this pull request Jul 23, 2022

Refactor iteration logic in the Flatten and FlatMap iterators #99541

Merged

Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2022

rustbot assigned joshtriplett and unassigned yaahc Dec 23, 2022

the8472 force-pushed the simplify-advance-by branch 2 times, most recently from 8b2cf20 to 3dbf7ac Compare March 13, 2023 19:08

scottmcm self-assigned this Mar 13, 2023

the8472 marked this pull request as ready for review March 13, 2023 19:09

the8472 changed the title ~~Change advance(_back)_by to return usize instead of Result<(), usize>~~ Change advance(_back)_by to return the remainder instead of the number of processed elements Mar 13, 2023

scottmcm reviewed Mar 13, 2023

View reviewed changes

library/core/src/iter/traits/double_ended.rs Outdated Show resolved Hide resolved

scottmcm reviewed Mar 13, 2023

View reviewed changes

library/core/src/ops/index_range.rs Show resolved Hide resolved