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

Stabilize step_by for 1.2.0 #25798

Closed
wants to merge 1 commit into from

Conversation

krzysz00
Copy link
Contributor

step_by is generally useful (it was used in multiple places in the
standard library). When it landed in mid-March, it was marked unstable
because it was a "recent addition" to the API. It's been two months, and
there seems to be no controversy about its inclusion in Rust. Therefore,
I propose that we stabilize step_by.

@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 @brson (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 CONTRIBUTING.md for more information.

@killercup
Copy link
Member

There was a discussion about step_by on Reddit recently, bringing up some good points:

It's not decided yet whether a negative step will be allowed and the two methods [step_by and rev] don't work together currently.

This is also #23588. Is any of this resolved? Does stabilizing hinder any progress on this?

@krzysz00
Copy link
Contributor Author

I guess that we could make negative numbers in step_by undefined, so that we can fix the "counting backwards" issue either by defining what negatives do or adding an impl for DoubleEndedIter. Neither of those would break backwards compatibility.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 26, 2015
@alexcrichton
Copy link
Member

As @killercup mentioned, the main outstanding question preventing this API from stabilizing is deciding what to do about negative steps. Adding undefined behavior probably isn't something we want to do to a core iterator adaptor, so we'll need to make a decision on this one way or another.

There's also the question about the Step trait and what to do about it. There have been a few small bugfixes recently to the steps_between calculation (handling negative steps), and it may not be 100% bullet-proof just yet.

Just points to consider!

@krzysz00
Copy link
Contributor Author

Yeah, we may still want to wait to stabilize, since we don't have negative steps yet.

I think that negative steps should (conceptually) work like reversing the range and stepping over the reversed range with a positive step. (EDIT) I think that's how it works for two-sided ranges now, but not one-sided ranges.

@alexcrichton alexcrichton added the I-needs-decision Issue: In need of a decision. label Jun 2, 2015
@bors
Copy link
Contributor

bors commented Jun 7, 2015

☔ The latest upstream changes (presumably #26066) made this pull request unmergeable. Please resolve the merge conflicts.

step_by is generally useful (it was used in multiple places in the
standard library). When it landed in mid-March, it was marked unstable
because it was a "recent addition" to the API. It's been two months, and
there seems to be no controversy about its inclusion in Rust. Therefore,
I propose that we stabilize step_by.
@alexcrichton
Copy link
Member

Ok, after some discussion it seems like it may still be somewhat premature to stabilize this function just yet. We'd like to take some more time to talk about the interaction of step_by and negative steps. @aturon is going to write a discuss post about this soon, though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-needs-decision Issue: In need of a decision. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants