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

Incorrect lower bound from StepBy<A, RangeInclusive<A>>::size_hint #41477

Closed
scottmcm opened this issue Apr 23, 2017 · 1 comment
Closed

Incorrect lower bound from StepBy<A, RangeInclusive<A>>::size_hint #41477

scottmcm opened this issue Apr 23, 2017 · 1 comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

Repro: https://is.gd/nFIian (tried 2017-04-21)

#![feature(step_by)]
#![feature(inclusive_range_syntax)]

fn main() {
    let i = (0...10).step_by(3);
    assert!(i.size_hint().0 <= i.count());
}

Looks like the implementation is adding one to the Range (exclusive) lower bound, which is incorrect when the size of the base range isn't a multiple of the step.

Thoughts on whether it'd be a better fix to just not add one (correct, but a less-helpful hint) or to add one to end before calling Step::steps_between (better hint, more math and more overflow cases)?

scottmcm added a commit to scottmcm/rust that referenced this issue May 20, 2017
Follow-up to rust-lang#41439 (comment)

While doing so, remove the now-unused `step`, `steps_between`, and `is_negative` methods from `Step`.

Mostly simple, but needed two interesting changes:
* Override `Iterator::size_hint` for `iter::StepBy` (so hints aren't lost)
* Override `Iterator::size_hint` for `ops::RangeFrom` (so `(0..).size_hint()` returns what `(0..).step_by(1).size_hint()` used to)
(It turns out that `(0..).step_by(d)` is used in a bunch of tests, from `cycle` to `vec_deque`.)

Incidentally fixes rust-lang#41477
@cuviper
Copy link
Member

cuviper commented Jun 9, 2017

From what I saw on #42514, the upper bound can also be one too high. That's less problematic, I suppose, although the proposal to "just not add one" should not apply to the upper bound since then it would be too low in some cases.

Of course, it's a bit moot since this is now deprecated and expected to be removed soon. :)

@Mark-Simulacrum Mark-Simulacrum added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 23, 2017
bors added a commit that referenced this issue Jul 4, 2017
Delete deprecated & unstable range-specific `step_by`

Using the new one is annoying while this one exists, since the inherent method hides the one on iterator.

Tracking issue: #27741
Replacement: #41439
Deprecation: #42310 for 1.19
Fixes #41477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants