Let str::replace take a pattern #29498
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| let data = "abcdαβγδabcdαβγδ"; | ||
| assert_eq!(data.replace("dαβ", "😺😺😺"), "abc😺😺😺γδabc😺😺😺γδ"); | ||
| assert_eq!(data.replace('γ', "😺😺😺"), "abcdαβ😺😺😺δabcdαβ😺😺😺δ"); | ||
| assert_eq!(data.replace(&['a', 'γ'], "😺😺😺"), "😺😺😺bcdαβ😺😺😺δ😺😺😺bcdαβ😺😺😺δ"); |
There was a problem hiding this comment.
Cause of travis failure. Rust can’t auto-deref this array into closure because there’s nothing that explicitly requests a slice.
a96dfdc
to
6211213
Compare
|
Sorry about that. Probably fixed now. (I can't run tests locally at the moment because of #29492.) |
|
With #26870 (Default Type Parameter Fallback) enabled by default, this could avoid being breaking. Not sure if it's worth it to wait. Maybe there's a case where it's breaking anyway, but basically type param fallback “opens the floodgates” to generalizing API. |
|
This seems good to me, although I would like to have a crater run ahead of time with this to ensure we don't have much unintended breakage. If we have to wait for #26870 to get turned on by default that's also probably not the end of the world! |
|
I'm fine with waiting for default type parameter fallback if you prefer. If you decide to wait I'll split the doc fixes into a separate pull request |
|
☔ The latest upstream changes (presumably #30057) made this pull request unmergeable. Please resolve the merge conflicts. |
It appears this was left out of RFC rust-lang#528 because it might be useful to also generalize the second argument in some way. That doesn't seem to prevent generalizing the first argument now, however. This is a [breaking-change] because it could cause type-inference to fail where it previously succeeded.
6211213
to
34fe201
Compare
|
Rebased. |
|
A crater report reports 4 regressions, but I think they're all spurious and unrelated to this change. I was thinking about this though and the "most generalized" form of the API here would perhaps be to have the replacement either be In some sense though this is getting pretty close to just an API around regexes, however, so we may not want to go too far down that route. Regardless I'm gonna tag this with T-libs so we can talk about it in the libs triage meeting. |
|
Ok, default type parameter fallback seems to not be likely to arrive any time soon (unfortunately), so we should continue? |
|
We discussed this in the libs team meeting this week, and are happy to go forward, pending an additional crater run. |
|
Crater reports one regression which is just spurious, so I think this is good to go. |
|
⌛ Testing commit 34fe201 with merge 6fe73d4... |
|
💔 Test failed - auto-linux-64-opt |
|
@bors: retry On Tue, Jan 12, 2016 at 2:49 PM, bors [email protected] wrote:
|
|
⌛ Testing commit 34fe201 with merge f40932c... |
|
💔 Test failed - auto-mac-32-opt |
|
@bors: retry On Tuesday, January 12, 2016, bors [email protected] wrote:
|
|
⌛ Testing commit 34fe201 with merge 8796e01... |
It appears this was left out of RFC rust-lang/rfcs#528 because it might be useful to also generalize the second argument in some way. That doesn't seem to prevent generalizing the first argument now, however. This is a [breaking-change] because it could cause type-inference to fail where it previously succeeded. Also update docs for a few other methods that still referred to `&str` instead of patterns.
It appears this was left out of RFC rust-lang/rfcs#528 because it might be useful to
also generalize the second argument in some way. That doesn't seem to
prevent generalizing the first argument now, however.
This is a [breaking-change] because it could cause type-inference to
fail where it previously succeeded.
Also update docs for a few other methods that still referred to
&strinstead of patterns.