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

chunk like Iterators should panic on overflow #51254

Closed
Kerollmops opened this issue May 31, 2018 · 3 comments
Closed

chunk like Iterators should panic on overflow #51254

Kerollmops opened this issue May 31, 2018 · 3 comments

Comments

@Kerollmops
Copy link
Contributor

Kerollmops commented May 31, 2018

Currently the Chunks, ChunksMut, ExactChunks and ExactChunksMut adapters does return None if an overflow occur in the Iterator::nth method.

I think this is not a good thing to have a silent "error", we must be consistent with other iterator adapters like enumerate and panic if an overflow is detected.

This has been talked in the tracking issue of the ExactChunks/Mut iterators adapters.

@Kerollmops Kerollmops changed the title chunk like Iterators must panic on overflow chunk like Iterators should panic on overflow May 31, 2018
@stokhos stokhos added A-diagnostics Area: Messages for errors, warnings, and lints A-iterators Area: Iterators and removed A-diagnostics Area: Messages for errors, warnings, and lints A-iterators Area: Iterators labels Jun 1, 2018
@cuviper
Copy link
Member

cuviper commented Jun 4, 2018

https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.nth

nth() will return None if n is greater than or equal to the length of the iterator.

None seems to be expected. A multiplication overflow here is just an internal side effect of requesting a too-large n, but I don't see why this should panic. We could equally check for n >= self.len() before attempting the multiplication, but that's actually more computational work.

@Kerollmops
Copy link
Contributor Author

Kerollmops commented Jun 4, 2018

I agree with what you said.

The thing that bothered me is the fact that if it is not possible to access the chunk that produced an overflow with one nth call so it seems inconsistent to me that it can be accessed with two nth calls one after the other.

fn nth(&mut self, n: usize) -> Option<Self::Item> {
    let (start, overflow) = n.overflowing_mul(self.chunk_size);
    if start >= self.v.len() || overflow {
// ...

But it is not possible because if the multiplication has overflowed so it is likely that the accessed pointer is non addressable and so None must be returned.

So we can close this. Thank you for your time.

@cuviper
Copy link
Member

cuviper commented Jun 16, 2018

Closing as suggested.

@cuviper cuviper closed this as completed Jun 16, 2018
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

3 participants