Add `ascii::Char` (ACP#179) by scottmcm · Pull Request #111009 · rust-lang/rust

scottmcm · 2023-04-30T04:00:01Z

ACP second: rust-lang/libs-team#179 (comment)
New tracking issue: #110998

For now this is an enum as @kupiakos suggested, with the variants under a different feature flag.

There's lots more things that could be added here, and place for further doc updates, but this seems like a plausible starting point PR.

I've gone through and put an as_ascii next to every is_ascii: on u8, char, [u8], and str.

As a demonstration, made a commit updating some formatting code to use this: scottmcm@ascii-char-in-fmt (I don't want to include that in this PR, though, because that brings in perf questions that don't exist if this is just adding new unstable APIs.)

rustbot · 2023-04-30T04:00:06Z

r? @cuviper

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

rustbot · 2023-04-30T04:00:09Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

pitaj · 2023-04-30T04:42:59Z

Should ascii::Char implement TryFrom<u8> and TryFrom<char>?

scottmcm · 2023-04-30T04:56:00Z

@pitaj Eventually that seems perfectly reasonable. But that needs an error type, so I left them out of the first PR.

(We can always add more later, especially while it's unstable.)

ChayimFriedman2 · 2023-04-30T12:53:18Z

library/core/src/ascii/ascii_char.rs

This unsafe block can be avoided by doing char::from(self.as_u8()).

Good point! I should think more about my copy-pastes ;)

Updated.

Oh, wait, const. No char::from in a const fn. Changed this to a FIXME instead.

as casting works however:

Suggested change

unsafe { char::from_u32_unchecked(self as u32) }

self.as_u8() as char

Ah, right, yet another problem with as: I forget it works and it's not something with a signature in rustdoc to remind me 😬

ChayimFriedman2 · 2023-04-30T12:57:28Z

library/core/src/char/methods.rs

Why does this take &self and not self?

I agree that self would make more sense, but I think that for the existing is_ascii above too.

So I made this &self for consistency with the existing methods here. I'll add a note to the tracking issue to make sure that it's discussed further before stabilization.

ChayimFriedman2 · 2023-04-30T12:59:13Z

library/core/src/num/mod.rs

fbstj · 2023-05-01T08:28:48Z

library/core/src/ascii/ascii_char.rs

Suggested change

/// often known as the [ASCII]subset.

/// often known as the [ASCII] subset.

joboet · 2023-05-02T07:56:11Z

library/core/src/ascii/ascii_char.rs

as casting works however:

Suggested change

unsafe { char::from_u32_unchecked(self as u32) }

self.as_u8() as char

joboet · 2023-05-02T08:03:17Z

library/core/src/ascii/ascii_char.rs

Bikeshedding incoming: Why not call the methods from_byte/as_byte? There is no stable precedent in the standard library for including the numeric type name in methods and from_byte is IMHO much simpler to read.

There is, actually: https://doc.rust-lang.org/std/primitive.char.html#method.from_u32

So since it's char::from_u32, I figured that ascii::Char::from_u8 was the most consistent here.

But drop a note in the tracking issue if you feel strongly. I think that's a better place for bikeshedding discussions, so they'll be seen at (eventual) stabilization time.

I totally missed that one! Seems very reasonable to rhyme with it. Bikeshedding complete 😉.

BurntSushi · 2023-05-02T13:57:34Z

library/core/src/ascii/ascii_char.rs

Yeah I'm conflicted on this personally too. I agree with the decision at least in terms of this module internally, but I would personally very much like to just use use std::ascii; and ascii::Char instead of having to write out use std::ascii::AsciiChar;. It's an interesting stylistic point.

BurntSushi · 2023-05-02T14:01:00Z

library/core/src/ascii/ascii_char.rs

I'm fine with landing it like this, but I find that an enum like this is super weird. I'll post more of a comment on the tracking issue because I don't think I fully understand the trade offs here yet.

BurntSushi · 2023-05-02T14:01:28Z

library/core/src/ascii/ascii_char.rs

You don't have to do it now, but I would like to see a "Why would I use this?" section before stabilization. :-)

library/core/src/ascii/ascii_char.rs

ChayimFriedman2 · 2023-05-04T06:02:19Z

library/core/src/array/ascii.rs

+    #[inline]
+    pub const unsafe fn as_ascii_unchecked(&self) -> &[ascii::Char; N] {
+        let byte_ptr: *const [u8; N] = self;
+        let ascii_ptr = byte_ptr as *const [ascii::Char; N];


Here we can use cast(), right? We can also use the newly added ptr::from_ref().

scottmcm · 2023-05-04T16:28:18Z

I think this is ready for review for nightly now:
@rustbot ready

There's lots more to do here before it'd be ready for stabilization, but this is already 700+ lines, so I'd like to move those things (like putting examples on everything) into later PRs.

BurntSushi · 2023-05-04T16:46:38Z

Ship it. @bors r+

bors · 2023-05-04T16:46:40Z

📌 Commit 8c781b0 has been approved by BurntSushi

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#108865 (Add a `sysroot` crate to represent the standard library crates) - rust-lang#110651 (libtest: include test output in junit xml reports) - rust-lang#110826 (Make PlaceMention a non-mutating use.) - rust-lang#110982 (Do not recurse into const generic args when resolving self lifetime elision.) - rust-lang#111009 (Add `ascii::Char` (ACP#179)) - rust-lang#111100 (check array type of repeat exprs is wf) - rust-lang#111186 (Add `is_positive` method for signed non-zero integers.) - rust-lang#111201 (bootstrap: add .gitmodules to the sources) Failed merges: - rust-lang#110954 (Reject borrows of projections in ConstProp.) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned cuviper Apr 30, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 30, 2023

This comment has been minimized.

Sign in to view

scottmcm force-pushed the ascii-char branch from 5dcab78 to e5fd985 Compare April 30, 2023 04:26

ChayimFriedman2 reviewed Apr 30, 2023

View reviewed changes

library/core/src/num/mod.rs Outdated

Copy link

Contributor

ChayimFriedman2 Apr 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

scottmcm force-pushed the ascii-char branch from e5fd985 to 1cfb675 Compare April 30, 2023 18:21

This comment has been minimized.

Sign in to view

scottmcm force-pushed the ascii-char branch from 1cfb675 to 533e14e Compare April 30, 2023 18:25

fbstj reviewed May 1, 2023

View reviewed changes

scottmcm mentioned this pull request May 1, 2023

Add an "ascii character" type to reduce unsafe needs in conversions rust-lang/libs-team#179

Closed

joboet reviewed May 2, 2023

View reviewed changes

BurntSushi reviewed May 2, 2023

View reviewed changes

scottmcm 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 May 3, 2023

Add the basic ascii::Char type

8c781b0

scottmcm force-pushed the ascii-char branch from 533e14e to 8c781b0 Compare May 4, 2023 05:09

ChayimFriedman2 reviewed May 4, 2023

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 4, 2023

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 4, 2023

matthiaskrgr mentioned this pull request May 4, 2023

Rollup of 8 pull requests #111210

Merged

bors merged commit ea0b650 into rust-lang:master May 4, 2023

rustbot added this to the 1.71.0 milestone May 4, 2023

scottmcm deleted the ascii-char branch May 4, 2023 20:49

scottmcm mentioned this pull request May 4, 2023

Tracking Issue for ascii::Char (ACP 179) #110998

Open

7 tasks

	unsafe { char::from_u32_unchecked(self as u32) }
	self.as_u8() as char

	/// often known as the [ASCII]subset.
	/// often known as the [ASCII] subset.

Uh oh!

Conversation

scottmcm commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 30, 2023

Uh oh!

rustbot commented Apr 30, 2023

Uh oh!

This comment has been minimized.

pitaj commented Apr 30, 2023

Uh oh!

scottmcm commented Apr 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmcm commented May 4, 2023

Uh oh!

BurntSushi commented May 4, 2023

Uh oh!

bors commented May 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

scottmcm commented Apr 30, 2023 •

edited

Loading