Skip to content

Conversation

LukasKalbertodt
Copy link
Contributor

@LukasKalbertodt 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 kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 18, 2017
@kennytm
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

Ok done. Thanks!

@kennytm
Copy link
Member

kennytm commented Nov 18, 2017

r? @alexcrichton

This has been discussed in rust-lang#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.
The feature of those methods was renamed to "ascii_ctype_on_intrinsics".
@alexcrichton
Copy link
Member

@rfcbot fcp merge

Awesome, thanks @LukasKalbertodt!

@rfcbot
Copy link

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 rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 20, 2017
@rfcbot
Copy link

rfcbot commented Nov 21, 2017

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 21, 2017
@alexcrichton
Copy link
Member

@bors: r+

Thanks again @LukasKalbertodt!

@bors
Copy link
Collaborator

bors commented Nov 27, 2017

📌 Commit 0337017 has been approved by alexcrichton

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. beta-nominated Nominated for backporting to the compiler in the beta channel. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2017
@kennytm
Copy link
Member

kennytm commented Nov 27, 2017

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

@Mark-Simulacrum
Copy link
Member

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

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 28, 2017
@Mark-Simulacrum
Copy link
Member

@bors rollup

@kennytm
Copy link
Member

kennytm commented Nov 28, 2017

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

@Mark-Simulacrum
Copy link
Member

@bors r-

The changes didn't land in time for 1.23 and stabilizations won't
be backported to beta.
@LukasKalbertodt
Copy link
Contributor Author

@kennytm @Mark-Simulacrum changed it to 1.24

@kennytm
Copy link
Member

kennytm commented Nov 28, 2017

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Nov 28, 2017

📌 Commit c5aad96 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Nov 29, 2017

⌛ Testing commit c5aad96 with merge 1563cfc393ae744ab6a14152e86d3c8e773ff2fc...

@bors
Copy link
Collaborator

bors commented Nov 29, 2017

💔 Test failed - status-travis

@LukasKalbertodt
Copy link
Contributor Author

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

@kennytm
Copy link
Member

kennytm commented Nov 29, 2017

@bors retry — travis-ci/travis-ci#8821

kennytm added a commit to kennytm/rust that referenced this pull request Nov 29, 2017
…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
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
@LukasKalbertodt LukasKalbertodt deleted the stabilize-ascii-ctype branch May 3, 2020 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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