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

Move RangeArgument and Bound to libcore. #41460

Closed
wants to merge 1 commit into from

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Apr 22, 2017

This moves them to core::ops with the rest of the Range types and re-exports them in collections.

Right now, there's no reason why these shouldn't be accessible in libcore. Caution is due, however, because this would add Bound to libcore as stable immediately.

Also as a side note, it may be desirable in the future to split up libcore/ops.rs as it is getting rather big.

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Apr 22, 2017

Thanks for the PR! We’ll periodically check in on it to make sure that @aturon or someone else from the team reviews it soon.

It also looks like there are some compilation failures, so perhaps you could take a look at those?

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2017
@frewsxcv frewsxcv added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 22, 2017
@clarfonthey
Copy link
Contributor Author

I think that everything should be fixed now!

@Mark-Simulacrum
Copy link
Member

Looks like that's not quite true: some feature attributes need to be added to a few crates: https://travis-ci.org/rust-lang/rust/jobs/225394483#L780.

This is somewhat concerning, as that implies that moving this feature changed from stable to unstable? Or otherwise made things "now unstable," which feels like a regression? Might want to pay careful attention to the stability attributes... not certain.

@aturon
Copy link
Member

aturon commented Apr 24, 2017

cc @rust-lang/libs, I believe this change is in line with our general philosophy around libcore.

@alexcrichton
Copy link
Member

This seems reasonable to me, but note that it's adding a new insta-stable type, std::ops::Bound. Previously this was only available at std::collections::Bound and I don't think we have many other instances in the standard library of the same type available in two locations.

What's the rationale for the movement to justify the duplication?

@clarfonthey
Copy link
Contributor Author

I hadn't considered this, and you're right. I could make a core::collections module to hold this type, although it does make the most sense in core::ops.

@alexcrichton
Copy link
Member

@aturon it's been awhile since you last looked at this, mind giving it another look with an eye towards reviewing?

@Mark-Simulacrum
Copy link
Member

@clarcharr Looks like there are quite a few compilation failures due to feature gating, could you take a look?

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2017
@Mark-Simulacrum
Copy link
Member

@clarcharr Thanks for the contribution! The change looks good to me but it needs some changes still, see the Travis failure. Do you think you'll have a chance to make them soon?

@carols10cents
Copy link
Member

Hi @clarcharr, thank you for your PR! Since there hasn't been any activity on this recently, we're going to close this to keep our open PR list current, but please feel free to reopen this when you have time to work on this again ❤️

@clarfonthey clarfonthey deleted the rangeargument_core branch May 27, 2017 19:31
@clarfonthey
Copy link
Contributor Author

I've redone this in #42268 and #42269.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

7 participants