-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add String.replace_{first,last,all} #14436
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
Conversation
f0b30e7 to
c8b74ed
Compare
c8b74ed to
9ae28dc
Compare
nojb
left a comment
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.
LGTM
A second core dev approval is needed before merging.
9ae28dc to
1e8cfba
Compare
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.
The replace_all function is clearly useful and expected.
Overall, I agree with the labels for opposed arguments with a shared type.
I was a bit wary about the potential performance trap of replace_first or replace_last, but in fact the function are probably hard to use to emulate replace_all in an inefficient way (without resorting to testing physical equality). I am still not completely sure if those two functions would be really used a lot, but people would probably expect them to exist at least for symmetry's sake.
Globally, I agree that this PR should be part of the "String API improvement" bundle for 5.5 .
1e8cfba to
43142a2
Compare
I'm sure they will be, I can think of quite a few times where I had to replace a given suffix by something else (example here). Thanks for the approval. Rebased. |
43142a2 to
04de9e8
Compare
This PR adds the functions string replacing functions discussed in #14137 it is built (two last commits) on top of the PR of search primitives #14381.