-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Add String::replace_first
and String::replace_last
#134316
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
base: master
Are you sure you want to change the base?
Conversation
I don't think I see any libs-api feedback on the other thread, and that is from a while back anyway. Could you file an ACP for this at https://github.com/rust-lang/libs-team/issues? |
These methods would be convenient but I feel like they should probably come with an in-place |
This comment has been minimized.
This comment has been minimized.
ACP opened: rust-lang/libs-team#506 @rustbot label S-waiting-on-ACP I added |
☔ The latest upstream changes (presumably #135286) made this pull request unmergeable. Please resolve the merge conflicts. |
@zachs18 |
This is still waiting on the ACP |
@bors r+ rollup ACP approved, PR approved. Might be interested in the proposed |
@bors r- r=me when the conflicts are fixed. |
library/alloc/src/string.rs
Outdated
/// This method should be preferred over [`String::replacen(..., 1)`][replacen] | ||
/// as it can use the `String`'s existing capacity to prevent a reallocation if | ||
/// sufficient space is available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the wording here can be softened a bit to say this can (not should) be preferred over replacen
if you don't need to retain the original. At least while this is unstable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in latest push. Also updated to say string = string.replacen(..., 1);
instead of String::replacen(..., 1)
as replacen
isn't from String
, but str
, and also to show that this is only a replacement for replacen
if it was used to immediately replace the original String
.
Please also squash when you rebase, this PR has fixup commits |
0243f5c
to
8ed7fca
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
@rustbot ready |
library/alloc/src/lib.rs
Outdated
// | ||
// Library features: | ||
// tidy-alphabetical-start | ||
#![cfg_attr(not(no_global_oom_handling), feature(string_replace_in_place))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw I think you may as well drop the cfg_attr
and enable the feature unconditionally, usually we don't gate these too strictly. And we'll wind up using it elsewhere anyway.
I'll merge after PR CI finishes, with or without this (just ping me once it does if I miss it)
Rebased and modified by zachs18. Co-authored-by: zachs18 <[email protected]>
8ed7fca
to
271b10c
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
For more information how to resolve CI failures of this job, visit this link. |
Thanks! I assume the GCC failure is spurious @bors r=joshtriplett ,tgross35 |
@bors rollup |
Rebase of #97977 (cc @WilliamVenner)
The intra-doc link to
str::replacen
is a direct url-based link tostr::replacen
instd
's docs to work around #98941. This means that when building onlyalloc
's docs (and notstd
's), it will be a broken link. There is precedent for this e.g. incore::hint::spin_loop
which links tostd::thread::yield_now
using a url-based link and thus is a dead link when only buildingcore
's docs.ACP: rust-lang/libs-team#506