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 inclusive range (..=) #47813

Merged
merged 5 commits into from
Mar 15, 2018
Merged

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Jan 27, 2018

Stabilize the followings:

cc #28237
r? @rust-lang/lang

@kennytm
Copy link
Member Author

kennytm commented Jan 27, 2018

dotdoteq_in_patterns is included because the corresponding documentation PRs already mentioned it... If it turns out to be too early to stabilize I can take it out.

@kennytm kennytm added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2018
@leonardo-m
Copy link

See also: #45222

@kennytm
Copy link
Member Author

kennytm commented Jan 28, 2018

@leonardo-m The performance fix can be done independently from the stabilization I believe.

@bluss
Copy link
Member

bluss commented Jan 28, 2018

Performance might depend on the data fields available, stabilizing public fields in the range inclusive iterator is restricting possibilities (to be factual).

Having them not be iterators (but iterable) is an alternative, but the drawbacks of that are pretty clear from the original rust-lang/rfcs#1192, so it is not a good idea.

@nikomatsakis
Copy link
Contributor

The only way that it might be relevant is if we decided to change so that ..= doesn't implement Iterator but only IntoIterator. I've historically been mega-opposed, on the grounds that .. and ..= should behave as similarly as possible, but if that's truly needed for perf or something that might be persuasive... (I've no idea if that is part of #45222 or what)

@shepmaster
Copy link
Member

r? @withoutboats

@shepmaster
Copy link
Member

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned withoutboats Feb 3, 2018
@nrc
Copy link
Member

nrc commented Feb 4, 2018

I think we need documentation before we can stabilise things nowadays, and according to the tracking issue we don't have any of that.

@kennytm
Copy link
Member Author

kennytm commented Feb 4, 2018

@nrc:

The book and reference say they want to wait until stabilization, so are we entering deadlock 😂.

@steveklabnik
Copy link
Member

steveklabnik commented Feb 4, 2018

The book and reference say they want to wait until stabilization, so are we entering deadlock 😂.

Usually, the book and reference need a PR open, and then the stuff gets stabilized, and then they get merged. So we're still good :)

@nrc
Copy link
Member

nrc commented Feb 4, 2018

@bors: r+

Thanks for the links!

@bors
Copy link
Contributor

bors commented Feb 4, 2018

📌 Commit 570ffb2 has been approved by nrc

@bors
Copy link
Contributor

bors commented Feb 5, 2018

🔒 Merge conflict

@bors
Copy link
Contributor

bors commented Feb 5, 2018

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

@kennytm
Copy link
Member Author

kennytm commented Feb 5, 2018

Fixed merge conflict.

Should this need any FCP? #28237 didn't go through a stabilization FCP yet.

@kdy1
Copy link
Contributor

kdy1 commented Feb 5, 2018

Inclusive range syntax causes panic when it's passed to attribute proc macro (actually an ICE, but rustc lies)

alexcrichton/futures-await#55


Edit: It doesn't matter because the issue is already reported. (#47950, I searched with proc macro range, but couldn't find)

@kennytm
Copy link
Member Author

kennytm commented Feb 5, 2018

@kdy1 This issue has been filed as #47950.

@nikomatsakis
Copy link
Contributor

I would be in favor of stabilizing, but I think we do need an FCP.

@kennytm if you want to push this through, do you think you could write up a summary comment on the tracking issue #28237 ? In particular, I would like to see:

  • Major features we expect to work, and the tests that we have for those in the code base
  • Pointers to the docs (at least pending)
  • Anything else that seems relevant

The idea is that people with minimal context should be able to quickly see what is being stabilized. It is not meant to rehash every argument (in particular, I don't think we have to go into the tortured history of choosing a particular sigil here).

@bors
Copy link
Contributor

bors commented Feb 6, 2018

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

@bors bors 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 Feb 6, 2018
@bors
Copy link
Contributor

bors commented Mar 15, 2018

📌 Commit 939cfa2 has been approved by petrochenkov

@rust-lang rust-lang deleted a comment from bors Mar 15, 2018
@rust-lang rust-lang deleted a comment from bors Mar 15, 2018
@bors
Copy link
Contributor

bors commented Mar 15, 2018

⌛ Testing commit 939cfa2 with merge 5c375d69d2f600668849c42c9c7ba880bc38eefb...

@bors
Copy link
Contributor

bors commented Mar 15, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 15, 2018
@kennytm
Copy link
Member Author

kennytm commented Mar 15, 2018

@bors retry #43283

[01:33:27] test sync::mpsc::tests::stress_recv_timeout_two_threads ... ok
[01:33:31] test collections::hash::map::test_map::test_lots_of_insertions ... ok
[01:33:53] test process::tests::test_process_output_fail_to_start ... test process::tests::test_process_output_fail_to_start has been running for over 60 seconds


No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

@bors bors 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 Mar 15, 2018
@bors
Copy link
Contributor

bors commented Mar 15, 2018

⌛ Testing commit 939cfa2 with merge 3926453...

bors added a commit that referenced this pull request Mar 15, 2018
Stabilize inclusive range (`..=`)

Stabilize the followings:

* `inclusive_range` — The `std::ops::RangeInclusive` and `std::ops::RangeInclusiveTo` types, except its fields (tracked by #49022 separately).
* `inclusive_range_syntax` — The `a..=b` and `..=b` expression syntax
* `dotdoteq_in_patterns` — Using `a..=b` in a pattern

cc #28237
r? @rust-lang/lang
@bors
Copy link
Contributor

bors commented Mar 15, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 3926453 to master...

@jadbox
Copy link

jadbox commented Mar 16, 2018

is there docs for this?

@shepmaster
Copy link
Member

@jadbox do you mean like https://doc.rust-lang.org/std/ops/struct.RangeInclusive.html ? Those have been around for a while now.

bors added a commit that referenced this pull request Apr 9, 2018
Correct a few stability attributes

* `const_indexing` language feature was stabilized in 1.26.0 by #46882
* `Display` impls for `PanicInfo` and `Location` were stabilized in 1.26.0 by #47687
* `TrustedLen` is still unstable so its impls should be as well even though `RangeInclusive` was stabilized by #47813
* `!Send` and `!Sync` for `Args` and `ArgsOs` were stabilized in 1.26.0 by #48005
* `EscapeDefault` has been stable since 1.0.0 so should continue to show that even though it was moved to core in #48735

This could be backported to beta like #49612
zackmdavis added a commit to zackmdavis/rust that referenced this pull request Jun 26, 2018
These were stabilized in March 2018's rust-lang#47813, and are the Preferred Way
to Do It going forward (q.v. rust-lang#51043).
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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. 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.