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

Implement TryFrom<char> for u8 #84640

Merged
merged 1 commit into from
Jan 8, 2022
Merged

Implement TryFrom<char> for u8 #84640

merged 1 commit into from
Jan 8, 2022

Conversation

ids1024
Copy link
Contributor

@ids1024 ids1024 commented Apr 28, 2021

Previously suggested in rust-lang/rfcs#2854.

It makes sense to have this since char implements From<u8>. Likewise u32, u64, and u128 (since #79502) implement From<char>.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2021
@kennytm kennytm added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 28, 2021
@kennytm
Copy link
Member

kennytm commented Apr 28, 2021

r? @SimonSapin

@rust-highfive rust-highfive assigned SimonSapin and unassigned kennytm Apr 28, 2021
@SimonSapin
Copy link
Contributor

I would have preferred if we didn’t expose conversions between bytes and Unicode through generic conversion traits that do not make the choice of character encoding visible. However since we already have that same conversion the other way around, at this point that’s not a reason not to add this one.

Still, please expand the doc-comment to discuss encodings like https://doc.rust-lang.org/std/primitive.char.html#impl-From%3Cu8%3E does. In fact that can be mostly copy-pasted, with things like "encode" instead of "decode".

Also please have the docs on the impl rather than only the method. Some rustdoc views fold/hide the latter.

@ids1024 ids1024 force-pushed the u8_from_char branch 3 times, most recently from bf3b654 to b383d78 Compare April 28, 2021 19:58
@ids1024
Copy link
Contributor Author

ids1024 commented Apr 28, 2021

Updated the doc comment.

I would have preferred if we didn’t expose conversions between bytes and Unicode through generic conversion traits that do not make the choice of character encoding visible. However since we already have that same conversion the other way around, at this point that’s not a reason not to add this one.

Indeed. If From<u8> for char and such didn't already exist, it would be worth considering what function naming would most clearly match the behavior. But too late for that I suppose.

@SimonSapin
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 28, 2021

📌 Commit b383d7885ba7bcd0da670e4ba7ac2ec5ad76e618 has been approved by SimonSapin

@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 Apr 28, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Apr 28, 2021

Shouldn't this go through FCP first?

Regardless, the stable version here is set to 1.52.0, which is to be released next week. It should be 1.53 or 1.54 if this were to be merged now.

@bors r-

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 28, 2021
@ids1024
Copy link
Contributor Author

ids1024 commented Apr 28, 2021

Regardless, the stable version here is set to 1.52.0, which is to be released next week. It should be 1.53 or 1.54 if this were to be merged now.

Ah right. Updated that to 1.53.

@kennytm kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 29, 2021
@SimonSapin
Copy link
Contributor

Oh right, I forgot this is insta-stable.

As discussed above, this conversion complements the existing impl From<u8> for char and copies most of its docs regarding the semantics of this "identity" character encoding.

@rfcbot fcp merge

@joshtriplett
Copy link
Member

Let's try that again and see if rfcbot notices:

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 29, 2021

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

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 29, 2021
library/core/src/char/convert.rs Outdated Show resolved Hide resolved
library/core/src/char/convert.rs Outdated Show resolved Hide resolved
@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 2, 2021
@jyn514
Copy link
Member

jyn514 commented Oct 3, 2021

Hm, I guess the issue is that the relative path doesn't work because the docs appear on two pages (u8 and TryFrom).

This should be fixed if you use intra-doc links. Since #87073 rustdoc allows anchors in links to primitives, but you'll have to wait a few weeks for the bootstrap compiler to be bumped.

@jyn514
Copy link
Member

jyn514 commented Oct 3, 2021

Oh right, I forgot I already commented on this PR 🤦

@jyn514 jyn514 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2021
@rust-log-analyzer

This comment has been minimized.

@ids1024
Copy link
Contributor Author

ids1024 commented Oct 23, 2021

Updated the error type and the doc link, though that's still not working. Not sure what the schedule for bootstrap compile updates is.

@jyn514
Copy link
Member

jyn514 commented Oct 23, 2021

@ids1024 #90042

@ids1024
Copy link
Contributor Author

ids1024 commented Oct 27, 2021

Rebasing now that that PR is merged, CI is passing.

@m-ou-se
Copy link
Member

m-ou-se commented Dec 4, 2021

Sorry for the delay. This looks ready to merge now. Can you update the stabilization version to 1.59.0?

Previously suggested in rust-lang/rfcs#2854.

It makes sense to have this since `char` implements `From<u8>`. Likewise
`u32`, `u64`, and `u128` (since rust-lang#79502) implement `From<char>`.
@m-ou-se
Copy link
Member

m-ou-se commented Jan 8, 2022

@bors r+ p=1 rollup

@bors
Copy link
Contributor

bors commented Jan 8, 2022

📌 Commit a02639d has been approved by m-ou-se

@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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jan 8, 2022
@ehuss ehuss mentioned this pull request Jan 8, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#84640 (Implement `TryFrom<char>` for `u8`)
 - rust-lang#92336 (Remove &self from PrintState::to_string)
 - rust-lang#92375 (Consolidate checking for msvc when generating debuginfo)
 - rust-lang#92568 (Add note about non_exhaustive to variant_count)
 - rust-lang#92600 (Add some missing `#[must_use]` to some `f{32,64}` operations)
 - rust-lang#92610 (Create CSS class instead of using inline style for search results)
 - rust-lang#92632 (Implement stabilization of `#[feature(available_parallelism)]`)
 - rust-lang#92650 (Fix typo in `StableCrateId` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 83de77d into rust-lang:master Jan 8, 2022
@rustbot rustbot added this to the 1.59.0 milestone Jan 8, 2022
@bors
Copy link
Contributor

bors commented Jan 8, 2022

⌛ Testing commit a02639d with merge 84abaf3...

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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-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.