Add inherent constructors on str#131118
Add inherent constructors on str#131118robertbastian wants to merge 7 commits intorust-lang:masterfrom
str#131118Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #131269) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #131767) made this pull request unmergeable. Please resolve the merge conflicts. |
I think it'll make your life much easier to do it in different PRs. Add the new ones first, then you can do separate PRs to update different parts of things to use the new ones, then once you're through that you can PR the future-deprecation. (Keeping it locally will help you make those other PRs, but no need to check it in for a while.) |
|
@robertbastian |
|
Sorry I'm afk until January 🙃 |
|
@robertbastian
Do you have plans to proceed with this soon? |
|
Thanks for the bump, will try to get to this today. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| pub fn leak<'a>(self) -> &'a mut str { | ||
| let slice = self.vec.leak(); | ||
| unsafe { from_utf8_unchecked_mut(slice) } | ||
| unsafe { str::from_utf8_unchecked_mut(slice) } |
There was a problem hiding this comment.
There are a lot of places in this PR that change from std:str::* to the new inherent methods. Is there a reason for this? It would be much cleaner to add the inherent methods now, but don't replace any existing uses until a follow up PR (and once the feature is closer to stable).
There was a problem hiding this comment.
because I've tried to future-deprecation-warning them, so I already cleaned up all usages
There was a problem hiding this comment.
Don't do that in the same PR. Add the unstable API first, we can consider encouraging the newer APIs somehow once this is closer to stabilization - but certainly not while it is unstable.
There was a problem hiding this comment.
So that's basically undoing the whole PR.
| /// ``` | ||
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| #[rustc_const_stable(feature = "const_str_from_utf8_shared", since = "1.63.0")] | ||
| #[rustc_diagnostic_item = "str_from_utf8"] |
There was a problem hiding this comment.
Don't move the existing diagnostic items. This feature will be unstable for a while, we don't want to break everything that relies on these in the meantime.
| // FIXME(const-hack): This should use `?` again, once it's `const` | ||
| match run_utf8_validation(v) { | ||
| match super::run_utf8_validation(v) { | ||
| Ok(_) => { | ||
| // SAFETY: validation succeeded. | ||
| Ok(unsafe { from_utf8_unchecked_mut(v) }) | ||
| Ok(unsafe { str::from_utf8_unchecked_mut(v) }) | ||
| } | ||
| Err(err) => Err(err), |
There was a problem hiding this comment.
Can't this block be str::from_utf8_mut, rather than matching and calling str::from_utf8_unchecked_mut?
There was a problem hiding this comment.
maybe, I feel like there's a reason why it isn't but that's lost to time
There was a problem hiding this comment.
This test shouldn't be needed once the compiler feature is removed.
There was a problem hiding this comment.
Regarding the most recent test failure, it would be easiest for the new methods in this file to call the core::str::* functions rather than moving the implementations here.
There was a problem hiding this comment.
I'd like these to be the canonical implementations
There was a problem hiding this comment.
That shouldn't matter, and can change in a future PR if there is a reason. For now it is cleanest to touch the existing methods as little as possible.
| /// assert_eq!("💖", sparkle_heart); | ||
| /// ``` | ||
| #[unstable(feature = "inherent_str_constructors", issue = "131114")] | ||
| #[rustc_const_unstable(feature = "inherent_str_constructors", issue = "131114")] |
There was a problem hiding this comment.
rustc_const_unstable shouldn't be needed anymore for functions that are #[unstable(...)] and const
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
I'm closing this because I cannot be bothered. This is way more work than it should be. |
|
The initial change should only be additions to We should not be deprecating, migrating, or changing diagnostics for anything at the same time, especially without a stable replacement (yet). Attempting to do this at once is part of why the changes seems more complex than needed (doing so requires updates to the compiler and tests). |
#131114
Unresolved questions
Do we mark the
core::strfunctions as future-deprecation? This will causes warnings whencore::stris imported andstr::from_utf8is used, because the compiler decides to interpret that as the module, not the type.