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

Change trait RangeArgument to struct RangeBounds. #43033

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@clarfon
Copy link
Contributor

clarfon commented Jul 3, 2017

This follows from the discussion in #30877. This removes the RangeArgument trait in favour of the RangeBounds struct, where RangeArgument<T> is loosely translated to Into<RangeBounds<T>>.

I'm still working on testing this but I'll upload the PR now and see how people feel.

Additionally, I was going to at some point refactor the code for dealing with ranges of usize into methods on RangeBounds<usize>. That isn't blocking on this commit, but it's something that should be done at some point.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jul 3, 2017

r? @BurntSushi

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

@clarfon clarfon force-pushed the clarfon:range_bounds branch 4 times, most recently from 5e55907 to fbd095d Jul 3, 2017

@aidanhs

This comment has been minimized.

Copy link
Member

aidanhs commented Jul 5, 2017

Hi @clarcharr, looks like there has been a bunch of CI failures like

[00:01:59] error[E0549]: rustc_deprecated attribute must be paired with either stable or unstable attribute
[00:01:59]    --> /checkout/src/liballoc/lib.rs:214:1
[00:01:59]     |
[00:01:59] 214 | pub use core::ops::Bound;
[00:01:59]     | ^^^^^^^^^^^^^^^^^^^^^^^^^
[00:01:59] 
[00:01:59] error: use of unstable library feature 'range_argument' (see issue #30877)
[00:01:59]   --> /checkout/src/liballoc/btree/map.rs:16:24
[00:01:59]    |
[00:01:59] 16 | use core::ops::{Index, RangeBounds};
[00:01:59]    |                        ^^^^^^^^^^^
[00:01:59]    |
[00:01:59]    = help: add #![feature(range_argument)] to the crate attributes to enable

Is this PR still a work in progress (per your original comment)?

@clarfon

This comment has been minimized.

Copy link
Contributor Author

clarfon commented Jul 5, 2017

It is! I'll get back to this today or tomorrow.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 6, 2017

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

@brson brson added the beta-nominated label Jul 8, 2017

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jul 8, 2017

This needs to land, and backport, within a week or not at all. It is slated for stabilization on 7/20.

I am not crazy about making API changes at the last minute as this does. A rename - ok (sorta...), but changing how the API works seems unnecessarily risky.

The stabilization of this API is in this backport.

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Jul 8, 2017

I'm a bit confused about the API change, honestly. I see it was suggested #30877 (comment) but there wasn't much discussion. There was consensus around a rename though, but it seems weird to rush the other change -- and why would we backport it for a release before basically anyone has a chance to try it on nightly? Maybe there is more context that I missed?

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Jul 8, 2017

OK, I looked a little more closely at the situation.

  1. We decided in the tracking issue to stabilize RangeArgument, and pretty much everyone agreed to change the name but nobody got around to doing it.
  2. During FCP, @johncf suggested the "Into trick". Nobody picked it up at the time, indeed @Stebalien and @sfackler had gentle rebuttals, but @clarcharr adopted it for this PR.
  3. Right after stabilization, @glaebhoerl complained about the naming.

This PR changes the name and implements the Into trick but does not stabilize anything. The backport stabilizes the API without either of those.

Based on that contradiction it seems to me that we haven't agreed on the design of this feature. Should we just revert the stabilization for now?

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 8, 2017

Nothing related to this is being stabilized AFAIK.

@clarfon

This comment has been minimized.

Copy link
Contributor Author

clarfon commented Jul 8, 2017

@brson I should clarify that this is absolutely not expected to stabilise soon. The intention was to offer an implementation of the Into version as a POC to make sure that people are okay with it, let the dust settle, then stabilise it. Right now I still have a bit of work to do making BTree*::range work as expected, but I do feel like this is a better way forward.

If the team would prefer to leave RangeArgument as it is and simply rename it for the sake of getting this stabilised quicker, I can modify this PR to do that.

@liigo

This comment has been minimized.

Copy link
Contributor

liigo commented Jul 11, 2017

@durka

Based on that contradiction it seems to me that we haven't agreed on the design of this feature. Should we just revert the stabilization for now?

I think so.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jul 17, 2017

So I'm not clear what the status of this is. I don't think we're backporting this to beta at this point (it's too late).

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 19, 2017

There's no need to beta backport this - RangeArgument was un-stabilized.

This does seem like a reasonable way forward though. r=me with a rebase!

/// An intermediate trait for specialization of `Extend`.
#[doc(hidden)]
trait SpecExtend<I: IntoIterator> {
/// Extends `self` with the contents of the given iterator.
fn spec_extend(&mut self, iter: I);
}

#[rustc_deprecated(reason = "moved to core::ops", since = "1.19.0")]

This comment has been minimized.

@ollie27

ollie27 Jul 19, 2017

Contributor

This would need to be "1.21.0" now.

This comment has been minimized.

@clarfon

clarfon Jul 20, 2017

Author Contributor

Will do!

@@ -11,142 +11,7 @@
#![unstable(feature = "collections_range",
reason = "waiting for dust to settle on inclusive ranges",
issue = "30877")]
#![rustc_deprecated(reason = "moved to core::ops", since = "1.19.0")]

This comment has been minimized.

@ollie27

ollie27 Jul 19, 2017

Contributor

As this module only contained RangeArgument and that's been removed, this entire module can just be deleted along with its reexports in collections and std::collections.

This comment has been minimized.

@clarfon

clarfon Jul 20, 2017

Author Contributor

Will do!

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 27, 2017

pign @clarcharr, just wanted to make sure this stayed on your radar

@aidanhs

This comment has been minimized.

Copy link
Member

aidanhs commented Aug 3, 2017

@clarcharr I'm going to close this to keep the queue clean, but if you manage to get back to this to rebase let us know and we'll reopen!

@aidanhs aidanhs closed this Aug 3, 2017

@liigo liigo referenced this pull request Sep 12, 2017

Closed

relaunch `RangeBounds` #44518

@clarfon clarfon deleted the clarfon:range_bounds branch May 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.