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 some `ascii_ctype` methods #46077

Merged
merged 3 commits into from Nov 29, 2017

Conversation

Projects
None yet
6 participants
@LukasKalbertodt
Contributor

LukasKalbertodt commented Nov 18, 2017

As discussed in #39658, this PR stabilizes those methods for u8 and char. All inherent ascii_ctype for [u8] and str are removed as we prefer the more explicit version s.chars().all(|c| c.is_ascii_()).

This PR doesn't modify the AsciiExt trait. There, the ascii_ctype methods are still unstable. It is planned to remove those in the future (I think). I had to modify some code in ascii.rs to properly implement AsciiExt for all types.

Fixes #39658.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 18, 2017

Member

@LukasKalbertodt The current nightly version is 1.23.0, unless you mean to backport these to the current beta.

Member

kennytm commented Nov 18, 2017

@LukasKalbertodt The current nightly version is 1.23.0, unless you mean to backport these to the current beta.

@LukasKalbertodt

This comment has been minimized.

Show comment
Hide comment
@LukasKalbertodt

LukasKalbertodt Nov 18, 2017

Contributor

@kennytm Oops, nope, I confused things here. So I'll change my latest commit to since = "1.24.0", yes?

Contributor

LukasKalbertodt commented Nov 18, 2017

@kennytm Oops, nope, I confused things here. So I'll change my latest commit to since = "1.24.0", yes?

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 18, 2017

Member

Make them 1.23.0 for now. We can backport it to the next beta if it didn't make it.

Member

kennytm commented Nov 18, 2017

Make them 1.23.0 for now. We can backport it to the next beta if it didn't make it.

@LukasKalbertodt

This comment has been minimized.

Show comment
Hide comment
@LukasKalbertodt

LukasKalbertodt Nov 18, 2017

Contributor

Ok done. Thanks!

Contributor

LukasKalbertodt commented Nov 18, 2017

Ok done. Thanks!

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm
Member

kennytm commented Nov 18, 2017

LukasKalbertodt added some commits Nov 9, 2017

Remove inherent `ascii_ctype` methods from `str` and `[u8]`
This has been discussed in #39658. It's a bit ambiguous how those
methods work for a sequence of ascii values. We prefer users writing
`s.iter().all(|b| b.is_ascii_...())` explicitly.

The AsciiExt methods still exist and are implemented for `str`
and `[u8]`. We will deprecated or remove those later.
Stabilize `ascii_ctype` methods for `u8` and `char`
The feature of those methods was renamed to "ascii_ctype_on_intrinsics".
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 20, 2017

Member

@rfcbot fcp merge

Awesome, thanks @LukasKalbertodt!

Member

alexcrichton commented Nov 20, 2017

@rfcbot fcp merge

Awesome, thanks @LukasKalbertodt!

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Nov 20, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Nov 20, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Nov 21, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Nov 21, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 27, 2017

Member

@bors: r+

Thanks again @LukasKalbertodt!

Member

alexcrichton commented Nov 27, 2017

@bors: r+

Thanks again @LukasKalbertodt!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 27, 2017

Contributor

📌 Commit 0337017 has been approved by alexcrichton

Contributor

bors commented Nov 27, 2017

📌 Commit 0337017 has been approved by alexcrichton

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 27, 2017

Member

@rust-lang/libs Just to confirm — are we going to backport this to 1.23?

Member

kennytm commented Nov 27, 2017

@rust-lang/libs Just to confirm — are we going to backport this to 1.23?

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum Nov 28, 2017

Member

Unmarking as beta-nominated. We basically never want to backport stabilizations with current policy.

Member

Mark-Simulacrum commented Nov 28, 2017

Unmarking as beta-nominated. We basically never want to backport stabilizations with current policy.

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum
Member

Mark-Simulacrum commented Nov 28, 2017

@bors rollup

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 28, 2017

Member

The versions needs to be changed to 1.24 in this case.

Member

kennytm commented Nov 28, 2017

The versions needs to be changed to 1.24 in this case.

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum
Member

Mark-Simulacrum commented Nov 28, 2017

@bors r-

Change `since` attribute from ctype methods from 1.23 to 1.24
The changes didn't land in time for 1.23 and stabilizations won't
be backported to beta.
@LukasKalbertodt

This comment has been minimized.

Show comment
Hide comment
@LukasKalbertodt

LukasKalbertodt Nov 28, 2017

Contributor

@kennytm @Mark-Simulacrum changed it to 1.24

Contributor

LukasKalbertodt commented Nov 28, 2017

@kennytm @Mark-Simulacrum changed it to 1.24

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 28, 2017

Member

@bors r=alexcrichton

Member

kennytm commented Nov 28, 2017

@bors r=alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 28, 2017

Contributor

📌 Commit c5aad96 has been approved by alexcrichton

Contributor

bors commented Nov 28, 2017

📌 Commit c5aad96 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 29, 2017

Contributor

⌛️ Testing commit c5aad96 with merge 1563cfc...

Contributor

bors commented Nov 29, 2017

⌛️ Testing commit c5aad96 with merge 1563cfc...

bors added a commit that referenced this pull request Nov 29, 2017

Auto merge of #46077 - LukasKalbertodt:stabilize-ascii-ctype, r=alexc…
…richton

Stabilize some `ascii_ctype` methods

As discussed in #39658, this PR stabilizes those methods for `u8` and `char`. All inherent `ascii_ctype` for `[u8]` and `str` are removed as we prefer the more explicit version `s.chars().all(|c| c.is_ascii_())`.

This PR doesn't modify the `AsciiExt` trait. There, the `ascii_ctype` methods are still unstable. It is planned to remove those in the future (I think). I had to modify some code in `ascii.rs` to properly implement `AsciiExt` for all types.

Fixes #39658.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 29, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented Nov 29, 2017

💔 Test failed - status-travis

@LukasKalbertodt

This comment has been minimized.

Show comment
Hide comment
@LukasKalbertodt

LukasKalbertodt Nov 29, 2017

Contributor

This error doesn't look like it has anything to do with my changes... 😕 Right? What shall I do now?

Contributor

LukasKalbertodt commented Nov 29, 2017

This error doesn't look like it has anything to do with my changes... 😕 Right? What shall I do now?

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm
Member

kennytm commented Nov 29, 2017

kennytm added a commit to kennytm/rust that referenced this pull request Nov 29, 2017

Rollup merge of rust-lang#46077 - LukasKalbertodt:stabilize-ascii-cty…
…pe, r=alexcrichton

Stabilize some `ascii_ctype` methods

As discussed in rust-lang#39658, this PR stabilizes those methods for `u8` and `char`. All inherent `ascii_ctype` for `[u8]` and `str` are removed as we prefer the more explicit version `s.chars().all(|c| c.is_ascii_())`.

This PR doesn't modify the `AsciiExt` trait. There, the `ascii_ctype` methods are still unstable. It is planned to remove those in the future (I think). I had to modify some code in `ascii.rs` to properly implement `AsciiExt` for all types.

Fixes rust-lang#39658.

bors added a commit that referenced this pull request Nov 29, 2017

Auto merge of #46362 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #45969, #46077, #46219, #46287, #46293, #46322, #46323, #46330, #46354, #46356
- Failed merges:

@bors bors merged commit c5aad96 into rust-lang:master Nov 29, 2017

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

gerd2002 pushed a commit to MegatonHammer/rust that referenced this pull request Apr 2, 2018

Rollup merge of rust-lang#46077 - LukasKalbertodt:stabilize-ascii-cty…
…pe, r=alexcrichton

Stabilize some `ascii_ctype` methods

As discussed in rust-lang#39658, this PR stabilizes those methods for `u8` and `char`. All inherent `ascii_ctype` for `[u8]` and `str` are removed as we prefer the more explicit version `s.chars().all(|c| c.is_ascii_())`.

This PR doesn't modify the `AsciiExt` trait. There, the `ascii_ctype` methods are still unstable. It is planned to remove those in the future (I think). I had to modify some code in `ascii.rs` to properly implement `AsciiExt` for all types.

Fixes rust-lang#39658.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment