Add u8::from_ascii_to_digit

[gimbling-away]

Fixes #95969

[rust-highfive]

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 r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. 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

[rust-highfive]

r? @scottmcm

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

[gimbling-away]

r? rust-lang/libs-api

@rustbot label +T-libs-api -T-libs

[Stargateur]

That not what is asked, the point of to_digit() of char is to handle radix. This simple cast u8 into character. This requested feature is to skip the cast of u8 to char, it's simple a shortcut (that also remove the strange i32 of to_digit).

The signature the issue is looking for is:

const fn to_ascii_digit(self, radix: u8) -> Option<Self>;
// or 
const fn from_ascii_digit(self, radix: u8) -> Option<Self>;
// I prefer this one cause self is admited to be a ascii_digit we when to convert to u8 (as the issue is looking for)
// to_ascii_digit look more like the reverse of what the issue is looking for

panic for radix > 36.

[hniksic]

In addition to what Stargateur said, I suggest that the result be an option for consistency with to_digit, and because only one failure mode is possible.

[gimbling-away]

That not what is asked, the point of to_digit() of char is to handle radix. This simple cast u8 into character. This requested feature is to skip the cast of u8 to char, it's simple a shortcut (that also remove the strange i32 of to_digit).

The signature the issue is looking for is:

const fn to_ascii_digit(self, radix: u8) -> Option<Self>;
// or 
const fn from_ascii_digit(self, radix: u8) -> Option<Self>;
// I prefer this one cause self is admited to be a ascii_digit we when to convert to u8 (as the issue is looking for)
// to_ascii_digit look more like the reverse of what the issue is looking for

panic for radix > 36.

Ahh. My bad.

I'll look into this in a minute, thanks so much for the correction 👍

[gimbling-away]

@rustbot author

[gimbling-away]

[The force push was a squash]

[Stargateur]

that why I advice from_ascii_digit in #96110 (comment), I totally agree it's unclear

[gimbling-away]

From the name it's not clear to me what direction this conversion goes.

Does b'5'.to_ascii_digit() give me 5? Or does 5u8.to_ascii_digit() give me b'5'?

The 'to ascii' in the name makes me think this returns an ascii character (b'5'), but it seems to do the opposite.

Agreed, the name is a bit confusing.

Renaming to from_ascii_digit 👍🏻

[hniksic]

Renaming to from_ascii_digit

Please note that the new method is supposed to parallel char::to_digit(). Calling it from_ascii_digit() sounds like it does the inverse, and has the potential to introduce confusion.

[Stargateur]

@hniksic char::to_digit() make sense cause it's a char we convert to a digit u32, if we name this u8::to_digit it would be unclear what the function do, cause u8 is not a character, digit::to_digit() doesn't make sense. I think from_ form is better unless you have a better proposition.

[yaahc]

I'm reassigning this PR because I'm taking a break from the review rotation for a little while. Thank you for your patience.

r? rust-lang/libs-api

[hniksic]

@Stargateur I see your point. Maybe from_ascii_to_digit()? That would show what is being performed (conversion of ASCII byte to a digit in the specified base) and reference char::to_digit().

[gimbling-away]

from_ascii_to_digit SGTM

Although I'd like to hear other people's takes on it before changing it =D

[gimbling-away]

@rustbot author

[gimbling-away]

Renamed to from_ascii_to_digit

[gimbling-away]

@rustbot ready

@dtolnay Mind taking a look? :)

[dtolnay]

@rust-lang/libs-api:
@rfcbot fcp close

I think I would prefer not to add this method.

The behavior of the proposed method is that ascii.from_ascii_to_digit(radix) is equivalent to (ascii as char).to_digit(radix) i.e. "from_ascii" refers to u8 as char and "to_digit" refers to char::to_digit, except that the new method has some extra casts thrown on top. In particular the proposed method takes the radix as u8 (but must be at most 36) and returns its result as u8 (but will be at most radix-1) whereas the existing char::to_digit takes its radix as u32 (must be at most 36) and returns u32 (at most radix-1).

Overall the benefit gained by giving this a name from_ascii_to_digit is not compelling to me. I find u8 as char a clearer way to conceptualize what is involved in this operation. We take a byte, interpret it as an ascii code, and get its digit value in a particular radix if it's valid in that radix.

As a counterproposal to the contributor, if they are interested, I would suggest to explore what a more fleshed out core::ascii would look like. Is there a useful notion of "ascii digit" as a type, which is able to make common patterns of ascii-manipulating code clearer to express? Are const generics useful for constraining a radix on a digit? It might turn out that none of this results in adding anything to core::ascii because our existing set of methods turn out to be hard to beat, but IMO it's a more promising approach than adding additional u8 methods.

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

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.

[gimbling-away]

The behavior of the proposed method is that ascii.from_ascii_to_digit(radix) is equivalent to (ascii as char).to_digit(radix) i.e. "from_ascii" refers to u8 as char and "to_digit" refers to char::to_digit, except that the new method has some extra casts thrown on top. In particular the proposed method takes the radix as u8 (but must be at most 36) and returns its result as u8 (but will be at most radix-1) whereas the existing char::to_digit takes its radix as u32 (must be at most 36) and returns u32 (at most radix-1).

Just to make it clear, feedback regarding the implementation details, I'd be glad to consider and improve upon it. @dtolnay

However, I do agree that a more fleshed dedicated module to ASCII utilities might be better rather than pushing more methods into u8.

[scottmcm]

👍 to closing in favour of something else later.

Having an AsciiChar in core sounds great to me. I think having more intentional types would make a huge difference for this api -- as it is it's very unclear to me from the name exactly which kinds of digits it refers to. Since we already have char::{to|from}_digit, if there could be AsciiChar::{to|from}_digit that would seem like a nice way.

(Such a type could even have a validity invariant of being actually ascii...)