Skip to content

Add ASCII-related methods from u8 and MIN/MAX to core::ascii::Char #143467

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 13, 2025
Merged

Add ASCII-related methods from u8 and MIN/MAX to core::ascii::Char #143467

merged 2 commits into from
Aug 13, 2025
+611 −1

Conversation

ChaiTRex
Copy link
Contributor

@ChaiTRex ChaiTRex commented Jul 5, 2025 •
edited by rustbot
Loading

  • Add ASCII-related methods from u8 to core::ascii::Char.
  • Add core::ascii::Char::MIN and core::ascii::Char::MAX.

Tracking issue #110998.

Can someone please ping @rust-lang/libs-api? These additions were not in the original ACP (rust-lang/libs-team#179).

@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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 Jul 5, 2025
@ibraheemdev
Copy link
Member

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 5, 2025
@rustbot rustbot assigned BurntSushi and unassigned ibraheemdev Jul 5, 2025
@tgross35
Copy link
Contributor

tgross35 commented Jul 26, 2025 •
edited
Loading

Suggested an ACP on Zulip. A few notes

  • I don't know if we need to exactly replicate naming from char/u8 here. E.g. I think the compact and non-redundant names to_uppercase and is_digit outweighs consistency with to_ascii_uppercase and is_ascii_digit.
  • Similarly, I don't know when is_ascii would be needed except for somebody migrating from char/u8 to AsciiChar. In which case, they should remove whatever they're doing with the check rather than accidentally using this.
  • All of these could take self/Self rather than &self/&Self since it's Copy (and trivially so). Arguably it should have been the same for u8 and char, not sure what to do with consistency here.

@ChaiTRex ChaiTRex mentioned this pull request Jul 26, 2025
Closed
@tgross35
Copy link
Contributor

tgross35 commented Aug 5, 2025

ACP was accepted with changes, for that:
@rustbot author

r? tgross35

@rustbot rustbot 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 Aug 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 5, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot assigned tgross35 and unassigned BurntSushi Aug 5, 2025
@rustbot

This comment has been minimized.

Sign in to view
@ChaiTRex ChaiTRex force-pushed the ascii_char_is_ascii branch from 381b5b3 to 16b8f73 Compare August 5, 2025 18:46
@rustbot

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@rustbot rustbot added the has-merge-commits PR has merge commits, merge with caution. label Aug 5, 2025
@ChaiTRex ChaiTRex force-pushed the ascii_char_is_ascii branch from 67cd427 to d811581 Compare August 5, 2025 21:44
@rustbot rustbot removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. has-merge-commits PR has merge commits, merge with caution. labels Aug 5, 2025
@ChaiTRex
Copy link
Contributor Author

ChaiTRex commented Aug 6, 2025

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2025
tgross35
tgross35 approved these changes Aug 13, 2025
View reviewed changes
library/core/src/ascii/ascii_char.rs
without modifying the original"]
#[unstable(feature = "ascii_char", issue = "110998")]
#[inline]
pub fn escape_ascii(self) -> super::EscapeDefault {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting that the function on char is called escape_unicode, so keeping ascii in the name seems fine here

Copy link
Contributor Author

@ChaiTRex ChaiTRex Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To add to that, I left that in for that reason and also because it clearly distinguishes its use of ASCII escapes (\x04) rather than Unicode escapes (\u{4}). I should probably better document what output should be expected for that particular character by adding a few lines to the doctest in another pull request.

@tgross35
Copy link
Contributor

Looks good, thank you!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Aug 13, 2025

📌 Commit d811581 has been approved by tgross35

It is now in the queue for this repository.

@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 Aug 13, 2025
@Kobzol Kobzol mentioned this pull request Aug 13, 2025
Merged
bors added a commit that referenced this pull request Aug 13, 2025
@bors
Auto merge of #145334 - Kobzol:rollup-fs5a133, r=Kobzol
1c9952f 
Rollup of 11 pull requests

Successful merges:

 - #143467 (Add ASCII-related methods from `u8` and `MIN`/`MAX` to `core::ascii::Char`)
 - #144519 (Constify `SystemTime` methods)
 - #144642 (editorconfig: don't trim trailing whitespace in tests)
 - #144870 (Stabilize `path_file_prefix` feature)
 - #145269 (Deprecate RUST_TEST_* env variables)
 - #145274 (Remove unused `#[must_use]`)
 - #145289 (chore(ci): upgrade checkout to v5)
 - #145303 (Docs: Link to payload_as_str() from payload().)
 - #145308 (Adjust documentation of `dangling`)
 - #145320 (Allow cross-compiling the Cranelift dist component)
 - #145325 (Add `cast_init` and `cast_uninit` methods for pointers)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fb9cd24 into rust-lang:master Aug 13, 2025
10 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Aug 13, 2025
rust-timer added a commit that referenced this pull request Aug 13, 2025
@rust-timer
Unrolled build for #143467
4dafa65 
Rollup merge of #143467 - ChaiTRex:ascii_char_is_ascii, r=tgross35

Add ASCII-related methods from `u8` and `MIN`/`MAX` to `core::ascii::Char`

* Add ASCII-related methods from `u8` to `core::ascii::Char`.
* Add `core::ascii::Char::MIN` and `core::ascii::Char::MAX`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 approved these changes

Assignees

@tgross35 tgross35

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.91.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ChaiTRex @rustbot @ibraheemdev @tgross35 @bors @BurntSushi