Skip to content

size_hint should be able to return a lower bound that is infinite #18751

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

Closed
vks opened this issue Nov 7, 2014 · 9 comments
Closed

size_hint should be able to return a lower bound that is infinite #18751

vks opened this issue Nov 7, 2014 · 9 comments

Comments

@vks
Copy link
Contributor

vks commented Nov 7, 2014

Taken from the standard library:

impl<A: Add<A, A> + Clone> Iterator<A> for Counter<A> {
    [...]
    #[inline]
    fn size_hint(&self) -> (uint, Option<uint>) {
        (uint::MAX, None) // Too bad we can't specify an infinite lower bound
    }
}

Some iterators are infinite, size_hint should be able to return (None, None) for that. This warrants changing the type signature to (Option<uint>, Option<uint>).

@thestinger
Copy link
Contributor

It's not incorrect to return uint::MAX as the lower bound. It's completely valid and I don't see a reason to make this more complex than it already is. The upper bound is already a somewhat dubious feature.

@vks
Copy link
Contributor Author

vks commented Nov 8, 2014

While it is not incorrect for the purposes of allocation, we might want to use size_hint to check whether an iterator is finite. For example, methods that consume iterators like collect could panic instead of trying to allocate the maximal amount of memory.

@Gankra
Copy link
Contributor

Gankra commented Nov 8, 2014

@vks That should trigger a panic anyway. If it doesn't, it's a bug.

@thestinger
Copy link
Contributor

It will end up triggering an immediate out-of-memory error once #18726 is fixed, but it doesn't at the moment.

@vks
Copy link
Contributor Author

vks commented Nov 8, 2014

It does, but it is a generic capacity overflow:

let v: Vec<_> = repeat(1u32).collect();

gives

task '<main>' panicked at 'capacity overflow',
/Users/rustbuild/src/rust-buildbot/slave/nightly-mac/build/src/libcore/option.rs:315

We could have a 'nicer' more specific panic.

@thestinger
Copy link
Contributor

It does, but it is a generic capacity overflow:

That's because it's doing size_of::<u32>() * std::uint::MAX. A u8 would generate an out-of-memory condition, which will be guaranteed to be an immediate out-of-memory error by #18726. The overflow could also be considered an out-of-memory error so that's there's only 1 failure case.

We could have a 'nicer' more specific panic.

That would imply an extra branch in a performance critical code path, and a third failure case as part of the API. I don't think it offers much value in return.

@vks
Copy link
Contributor Author

vks commented Nov 8, 2014

In theory this could be done at compile time, then there would be no performance impact. (But then, we probably couldn't use size_hint.)

How is the panic message part of the API? You cannot catch panics. If you don't want want a different panic, it could also be a lint.

@thestinger
Copy link
Contributor

In theory this could be done at compile time, then there would be no performance impact.

It could sometimes be done at compile-time (type-based, and not catching instances that are possibly infinite at runtime), but Rust doesn't have the metaprogramming features to do that.

You cannot catch panics.

You can catch panics. They are exceptions, nothing more or less - Rust pays a very high cost to make it possible to catch them instead of aborting the process. There's currently the restriction of only being able to catch them at task boundaries for memory safety.

it could also be a lint.

I don't think library related stuff belongs in a lint built-in to the compiler. IMO, it's not a widely applicable warning anyway.

@steveklabnik
Copy link
Member

I'm pulling a massive triage effort to get us ready for 1.0. As part of this, I'm moving stuff that's wishlist-like to the RFCs repo, as that's where major new things should get discussed/prioritized.

This issue has been moved to the RFCs repo: rust-lang/rfcs#677

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

No branches or pull requests

5 participants