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 inherent str constructors #136517

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

m4rch3n1ng
Copy link
Contributor

implement #131114

this implements

  • str::from_utf8
  • str::from_utf8_mut
  • str::from_utf8_unchecked
  • str::from_utf8_unchecked_mut

i left std::str::from_raw_parts and std::str::from_raw_parts_mut out of this as those are unstable and were not mentioned by the tracking issue or the original pull request, but i can add those here as well.

i was also unsure of what to do with the rustc_const_(un)stable attributes: i removed the #[rustc_const_stable] attribute from str::from_utf8, str::from_utf8_unchecked and str::from_utf8_unchecked_mut, and left the#[rust_const_unstable] in str::from_utf8_mut (btw why is that one not const stable yet with #57349 merged?).

is there a way to redirect users to the stable std::str::from_utf8 instead of only saying "hey this is unstable"?

for now i just removed the check for str::from_utf8 in the test in tests/ui/suggestions/suggest-std-when-using-type.rs.

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jhpratt (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 3, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

i am not quite sure how this failure is in any way related to this pr,
since i am only touching inherent functions on str? but sure.
@jhpratt
Copy link
Member

jhpratt commented Feb 5, 2025

i left std::str::from_raw_parts and std::str::from_raw_parts_mut out of this as those are unstable and were not mentioned by the tracking issue or the original pull request, but i can add those here as well.

That can be done in a separate PR if you wish!

i was also unsure of what to do with the rustc_const_(un)stable attributes: i removed the #[rustc_const_stable] attribute from str::from_utf8, str::from_utf8_unchecked and str::from_utf8_unchecked_mut, and left the#[rust_const_unstable] in str::from_utf8_mut (btw why is that one not const stable yet with #57349 merged?).

The attributes are correct. The compiler will (thankfully!) complain if they're clearly wrong, but there are edge cases that reviewers can keep an eye out for if you're ever not sure. As to why the mutable version is not const stable, that's a great question. I asked in the tracking issue, so the team should look at it soon-ish.

is there a way to redirect users to the stable std::str::from_utf8 instead of only saying "hey this is unstable"?

Probably, but don't worry about it. I'll ping @estebank to see if there's anything that can be done here (ideally for the general case, as there's a number of things like this).


Thank you for doing this! It looks good to me, so merging 🙂

@bors r+

@bors
Copy link
Contributor

bors commented Feb 5, 2025

📌 Commit 15adc38 has been approved by jhpratt

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 Feb 5, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2025
Rollup of 12 pull requests

Successful merges:

 - rust-lang#132547 (cg_gcc: Directly use rustc_abi instead of reexports)
 - rust-lang#135572 (tests: Port `split-debuginfo` to rmake.rs)
 - rust-lang#135964 (Make cenum_impl_drop_cast a hard error)
 - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe})
 - rust-lang#136304 (Reject negative literals for unsigned or char types in pattern ranges and literals)
 - rust-lang#136418 (uefi: process: Add support for command environment variables)
 - rust-lang#136449 (std: move network code into `sys`)
 - rust-lang#136517 (implement inherent str constructors)
 - rust-lang#136536 (Rename and Move some UI tests to more suitable subdirs)
 - rust-lang#136537 (Update `compiler-builtins` to 0.1.145)
 - rust-lang#136555 (Rename `slice::take...` methods to `split_off...`)
 - rust-lang#136567 (Arbitrary self types v2: recursion test)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2025
Rollup of 12 pull requests

Successful merges:

 - rust-lang#132547 (cg_gcc: Directly use rustc_abi instead of reexports)
 - rust-lang#135572 (tests: Port `split-debuginfo` to rmake.rs)
 - rust-lang#135964 (Make cenum_impl_drop_cast a hard error)
 - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe})
 - rust-lang#136304 (Reject negative literals for unsigned or char types in pattern ranges and literals)
 - rust-lang#136418 (uefi: process: Add support for command environment variables)
 - rust-lang#136449 (std: move network code into `sys`)
 - rust-lang#136517 (implement inherent str constructors)
 - rust-lang#136536 (Rename and Move some UI tests to more suitable subdirs)
 - rust-lang#136537 (Update `compiler-builtins` to 0.1.145)
 - rust-lang#136555 (Rename `slice::take...` methods to `split_off...`)
 - rust-lang#136567 (Arbitrary self types v2: recursion test)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7ad1a3b into rust-lang:master Feb 6, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 6, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2025
Rollup merge of rust-lang#136517 - m4rch3n1ng:inherent-str-constructors, r=jhpratt

implement inherent str constructors

implement rust-lang#131114

this implements
- str::from_utf8
- str::from_utf8_mut
- str::from_utf8_unchecked
- str::from_utf8_unchecked_mut

i left `std::str::from_raw_parts` and `std::str::from_raw_parts_mut` out of this as those are unstable and were not mentioned by the tracking issue or the original pull request, but i can  add those here as well.

i was also unsure of what to do with the `rustc_const_(un)stable` attributes: i removed the `#[rustc_const_stable]` attribute from `str::from_utf8`, `str::from_utf8_unchecked` and `str::from_utf8_unchecked_mut`, and left the`#[rust_const_unstable]` in `str::from_utf8_mut` (btw why is that one not const stable yet with rust-lang#57349 merged?).

is there a way to redirect users to the stable `std::str::from_utf8` instead of only saying "hey this is unstable"?

for now i just removed the check for `str::from_utf8` in the test in `tests/ui/suggestions/suggest-std-when-using-type.rs`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants