Add method String::retain

[murarth]

Behaves like Vec::retain, accepting a predicate FnMut(char) -> bool
and reducing the string to only characters for which the predicate
returns true.

[rust-highfive]

r? @BurntSushi

(rust_highfive has picked a reviewer for you, use r? to override)

[murarth]

The motivation for this method is similar to that of Vec::retain. It transforms in-place for efficiency. It would be useful when one wants to strip a set of illegal characters from an input string.

[F001]

In HashMap::retain, we used F: FnMut(&K, &mut V) -> bool, in case we need to mutate the value in iteration. @alexcrichton also thinks it is better if we can improve Vec::retain to be F: FnMut(&mut T) -> bool, though it is too late to do this. So I guess it could be better if we use F: FnMut(&mut char)->bool.

[murarth]

For Vec and HashMap, it's trivial to pass a &mut instead of &. For String, however, the code would have to be changed to write the decoded char back into the buffer. Because the new char could be larger than the original, it would significantly increase the complexity and perhaps require the allocation of a second buffer to manage it. I don't think that's the right direction to go in, in this case.

[F001]

I just noticed that we can't just simply modify chars in place. It is not worth to do this. Please ignore my comments.

[aidanhs]

I've pinged @BurntSushi on irc for a review - we'll check in now and again to make sure this gets reviewed soon @murarth!

[BurntSushi]

Sorry I missed this! This seems like a reasonable API to have to me. With that said, it does seem like &mut char would be a nice thing to have if possible. It seems like it should be possible to implement, but would certainly be more complex than the implementation in this PR. Would it also entail a performance trade off too?

cc @rust-lang/libs

[murarth]

@BurntSushi: I wrote a &mut char implementation and a few benchmarks. Here's the comparison doing no mutation on the mutate-incapable versus mutate-capable implementations:

test bench_retain_alpha_simple              ... bench:         149 ns/iter (+/- 0)
test bench_retain_alpha_mutate_check        ... bench:         238 ns/iter (+/- 18)

Here's the full implementation: https://gist.github.com/murarth/ac22d58c75602ceb1cc5f79e87474c37

Edit: Updated benchmark results. Ha ha, forgot to compile the benchmarks with -O. I'm used to cargo bench doing that for me.

[arielb1]

Hi @BurntSushi - are you reviewing this? Just a friendly ping to keep this on your radar.

[carols10cents]

ping for review @BurntSushi @rust-lang/libs !

[alexcrichton]

Ah sorry for the delay @murarth! I think that not having &mut char here makes sense to me at least because there's nothing to get a reference to, and mutation seems best done through other APIs perhaps. @murarth want to file a tracking issue for this? This is still unstable so we can debate &mut char as well on the tracking issue, and otherwise this seems ok to land to me.

[murarth]

@alexcrichton: Created tracking issue #43874 and updated the code.

[leonardo-m]

Is a self.slice_unchecked(idx, len).chars().next().unwrap() followed by a ch.len_utf8() generating a small enough amount of code for the optimizer to optimize away? Or is it better to use/add something leaner that gets compiled faster?

[alexcrichton]

@bors: r+

Thanks @murarth! I figure we can probably update perf and such as this is unstable, so no need to block longer!

[bors]

📌 Commit 618ac89 has been approved by alexcrichton

[bors]

⌛ Testing commit 618ac89 with merge 82be83c...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 82be83c to master...