Slice retain

[MarinPostma]

adds retain method on slice. This meant moving the retain implementation from Vec to slice and having Vec's retain method rely on slice's.

tracking issue: #71831

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @shepmaster (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Mark-Simulacrum]

The slice retain method not dropping the values removed seems like an odd choice -- I would then guess that the user wanted to use Iterator::partition_in_place?

Maybe we can update the Vec::retain method to use that iterator method? We should benchmark that. I would not be opposed to adding a #[doc(alias)] attribute on the iterator for retain so that people can find it when looking for a slice retain, hopefully. (Or better, I guess, a pointer from the Vec::retain docs).

[MarinPostma]

@Mark-Simulacrum I share your concern, although, the pointer to the original slice remains valid, what would that mean if someone tried to access those dropped elements? We don't have that issue with Vec as the discarded elements are not accessible anyway.

Also the doc for partition_in_place does not maintain the relative order of the partitioned items, whereas the slice and vex method does

[shepmaster]

what would that mean if someone tried to access those dropped elements

What I took away from that comment is that this function should not be called retain because it doesn't drop values, not that it should attempt to drop the values.

[Mark-Simulacrum]

Yes, that's what I meant, sorry, I should have been more clear. Because of that I don't think retain is viable on slices (since you can't really "shrink" them, just move things around in them).

[shepmaster]

Also the doc for partition_in_place does not maintain the relative order of the partitioned items, whereas the slice and vex method does

Perhaps that means that the order should be fixed.

See also dedup_in_place. There was at least one comment that would have benefited from a known order.

[MarinPostma]

Yes, that's what I meant, sorry, I should have been more clear. Because of that I don't think retain is viable on slices (since you can't really "shrink" them, just move things around in them).

yes this makes sense. Whatever we decide to call it, we should link it to Vec::retain, so people looking for the slice counterpart can find it easily.

See also dedup_in_place. There was at least one comment that would have benefited from a known order.

Yes, I saw that, but it's referring to the duplicates order, the retain method does not provide any guarantees on the discarded elements order either. Do you think that we should pick another algorithm that guaranties that both part's relative order is preserved?

If it occurs that people have need for both parts, we could also return both, as was originally proposed for partition_dedup, this was already discussed, and it was deemed either too niche or too complicated.

[MarinPostma]

Interestingly, Iterator::partition_in_place is about twice faster than retain in small benchmarks I made

[shepmaster]

Do you think that we should pick another algorithm that guaranties that both part's relative order is preserved?

Possibly. There's multiple things I can see:

1. first partition stays ordered, second partition is undefined
2. first partition is undefined, second partition is undefined
3. first partition stays ordered, second partition stays ordered
4. first partition is undefined, second partition stays ordered

I'd say that 4 is not useful (and could probably be done by inverting the condition on 1).

The "second partition stays ordered" versions are only useful for the forms that make use of the second partition (e.g. not retain).

[MarinPostma]

This seems to be another issue though. The goal of this PR is to add a retain-like method on mutable slices. We'd expect it to work like it's Vec counterpart, thus preserving the order of the retained elements. I agree that there need to be a discussion about the name, as it does not drop the discarded elements, and maybe Iterator partition_in_place and retain will share implementation, once behavior is decided. Vec::retain is stable, and it's behavior is well defined, it seems natural to be able to do the same on slices.

Maybe we can update the Vec::retain method to use that iterator method? We should benchmark that.

It seems that partition_in_place is faster, at the cost of weaker guarantees, so maybe it makes sense that both method coexist, as preserved order is a guarantee that may be required (e.g. working on ordered set).

[bors]

☔ The latest upstream changes (presumably #72747) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

Ping from triage:
@MarinPostma - can you please resolve the merge conflicts, thank you.

[MarinPostma]

Yes sure

[JohnCSimon]

Ping from triage:
@MarinPostma - sorry there's another merge conflict. If this is ready for review, please ping shepmaster. thanks

[crlf0710]

Closing due to inactivity. Feel free to reopen or create a new PR when you have time. (Actually i suggest creating a zulip topic to finish up the API design first in this case.) Thanks!