Skip to content
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

Add retain to IdSlab. #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sebcrozet
Copy link

This allows the removal of elements, depending on a predicate, similar to Vec::retain.

This allows the removal of elements, depending on a predicate, similar to `Vec::retain`.
Copy link
Owner

@cristicbz cristicbz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Didn't realise anyone other than me uses this. Could you please add a few tests as well? I'm thinking at least

  1. retain on empty slab
  2. retain where everything is removed: check old ids don't work anymore and that when some new elements are added they work.
  3. Calling retain a few times where some things are removed each time; old ids don't work, newly added things do work.

/// assert!(!id_slab.contains(id3));
/// assert!(id_slab.contains(id4));
/// ```
pub fn retain<F: FnMut(&T) -> bool>(&mut self, mut f: F) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could take &mut T as argument; you can see the conversation here: https://github.com/Amanieu/rfcs/blob/retain_mut/text/0000-vec-retain_mut.md (in short there's no reason not to do this, it's strictly more general.

@cristicbz
Copy link
Owner

If you don't mind I can add the commit to do that, I'll leave your commit in anyway for authorship, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants