Skip to content

Specialize StepBy::nth - #47552

Merged
bors merged 6 commits into
rust-lang:masterfrom
oberien:stepby-nth
Jan 31, 2018
Merged

Specialize StepBy::nth#47552
bors merged 6 commits into
rust-lang:masterfrom
oberien:stepby-nth

Conversation

@oberien

@oberienoberien commented Jan 18, 2018

Copy link
Copy Markdown
Contributor

This allows optimizations of implementations of the inner iterator's .nth method.

@rust-highfive

rust-highfive commented Jan 18, 2018

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kimundi (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. Due to 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.

scottmcm
scottmcm reviewed Jan 19, 2018
View reviewed changes
Comment threadsrc/libcore/iter/mod.rs Outdated
}
n -= 1;
}
self.iter.nth(n * self.step)

@scottmcmscottmcmJan 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good change! ❤️

I think that this needs to handle overflow in the multiplication, though, since I could write (0u64..).step_by(0x10000).nth(0x10000) on a 32-bit machine, for example.

Also, tests?

@oberienoberienJan 19, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Tests are a good call, I found 2 off-by-ones while implementing them.

I added overflow behaviour, but it is pretty weighty. While I think that it shouldn't influence normal execution due to intrinsics::likely and branch prediction, I do think that it is pretty complicated code, which doesn't really add too much value. Anyway, now that it exists, I don't mind keeping it :)

varkor
varkor reviewed Jan 19, 2018
View reviewed changes
Comment threadsrc/libcore/iter/mod.rs Outdated
// overflow handling
loop {
let mul = n.checked_mul(step);
if unsafe { intrinsics::likely(mul.is_some())} {

@varkorvarkorJan 19, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: space before }.

@shepmastershepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 20, 2018
@shepmaster

shepmaster commented Jan 20, 2018

Copy link
Copy Markdown
Member

Thanks @oberien ! We'll make sure to get you a first-class review from @Kimundi or another team member soon!

@kennytm

kennytm commented Jan 24, 2018

Copy link
Copy Markdown
Member

Review ping for you @Kimundi!

@kennytmkennytm added the T-libs-api [DEPRECATED; DO NOT USE] label Jan 31, 2018
@kennytm

kennytm commented Jan 31, 2018

Copy link
Copy Markdown
Member

Since @Kimundi haven't left any comment here, reassigning to another @rust-lang/libs team member.

r? @dtolnay

@rust-highfiverust-highfive assigned dtolnay and unassigned KimundiJan 31, 2018
dtolnay
dtolnay approved these changes Jan 31, 2018
View reviewed changes

@dtolnaydtolnay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@dtolnay

dtolnay commented Jan 31, 2018

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jan 31, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit 4a0da4c has been approved by dtolnay

@kennytmkennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2018
@kennytmkennytm mentioned this pull request Jan 31, 2018
Merged
kennytm added a commit to kennytm/rust that referenced this pull request Jan 31, 2018
@kennytm
Rollup merge ofrust-lang#47552- oberien:stepby-nth, r=dtolnay
af95302
Specialize StepBy::nth
This allows optimizations of implementations of the inner iterator's `.nth` method.
bors added a commit that referenced this pull request Jan 31, 2018
@bors
Auto merge of#47900- kennytm:rollup, r=kennytm
8ccab7e
Rollup of 16 pull requests
- Successful merges: #47838, #47840, #47844, #47874, #47875, #47876, #47884, #47886, #47889, #47890, #47891, #47795, #47677, #47893, #47895, #47552
- Failed merges:
@bors
bors merged commit 4a0da4c into rust-lang:masterJan 31, 2018
@bvinc

bvinc commented Feb 7, 2018

Copy link
Copy Markdown
Contributor

I know I'm late. But does anyone else think that this function is rather large to be marked inline?

@scottmcm

scottmcm commented Feb 9, 2018

Copy link
Copy Markdown
Member

@bvinc Well, it's on a generic impl, so it'll be an inlining candidate regardless. And that's good in general, since you really want .step_by(2).nth(4) to inline and const fold.

That said, it might be interesting to split out the overflow handling into a separate function, to make it easier for LLVM to inline the short easy normal path without the hard part.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scottmcmscottmcmscottmcm left review comments
@dtolnaydtolnaydtolnay approved these changes
+1 more reviewer
@varkorvarkorvarkor left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dtolnaydtolnay

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libs-api[DEPRECATED; DO NOT USE]

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@oberien@rust-highfive@shepmaster@kennytm@dtolnay@bors@bvinc@scottmcm@varkor@Kimundi