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

O(1) count,nth,last for slice::{Windows,Chunks,ChunksMut} #27652

Merged
merged 1 commit into from
Aug 13, 2015

Conversation

alex-ozdemir
Copy link
Contributor

Provides a custom implementation of Iterator methods count, nth, and last for the structures slice::{Windows,Chunks,ChunksMut} in the core module.

These implementations run in constant time as opposed to the default implementations which run in linear time.

Addresses Issue #24214

r? @aturon

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.


#[inline]
fn nth(&mut self, n: usize) -> Option<Self::Item> {
let end = self.size + n;
Copy link
Member

Choose a reason for hiding this comment

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

This addition may overflow. The methods for add with overflow might be appropriate, take the None case on overflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that that said module is flagged as unstable - does that present an obstacle for using in in an impl which is stable?
Alternatively I could handle this manually.

Copy link
Member

Choose a reason for hiding this comment

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

Not an obstacle! There are several ways to do this, you can use .checked_add for example, but i thought overflowing_add would be the correct one for this particular line.

(saturating_add produces usize::MAX instead of overflowing, but that's not correct for the comparison end > self.len() — usize::MAX is not guaranteed to be larger).

@alex-ozdemir
Copy link
Contributor Author

Addressed the well-raised concerns regarding overflow in the nth() methods.
Thanks @bluss!

I opted to use both checked and overflowing methods as I found convenient - is this seen as bad style?

@bluss
Copy link
Member

bluss commented Aug 12, 2015

Great! Handling overflow cases is of course an issue of correctness, not safety.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 12, 2015

📌 Commit b223abb has been approved by bluss

@bluss
Copy link
Member

bluss commented Aug 12, 2015

@bors r-

Actually, can you squash this together into just one commit? Since the commits partly change and change things back.

@bluss
Copy link
Member

bluss commented Aug 12, 2015

r=me with squash

Implemented count, nth, and last in constant time for Windows, Chunks,
and ChunksMut created from a slice.

Included checks for overflow in the implementation of nth().

Also added a test for each implemented method to libcoretest.

Addresses rust-lang#24214
@bluss
Copy link
Member

bluss commented Aug 12, 2015

Thank you! (No notifications go out from commit changes, need a message on the PR thread for that)

@bors r+

@bors
Copy link
Contributor

bors commented Aug 12, 2015

📌 Commit e09f83e has been approved by bluss

@alex-ozdemir
Copy link
Contributor Author

Cool - I'll remember to post after commits in the future.

@bors
Copy link
Contributor

bors commented Aug 13, 2015

⌛ Testing commit e09f83e with merge 021389f...

bors added a commit that referenced this pull request Aug 13, 2015
Provides a custom implementation of Iterator methods `count`, `nth`, and `last` for the structures `slice::{Windows,Chunks,ChunksMut}` in the core module.

These implementations run in constant time as opposed to the default implementations which run in linear time.

Addresses Issue #24214 

r? @aturon
@bors bors merged commit e09f83e into rust-lang:master Aug 13, 2015
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants