Add retain function

[Xaeroxe]

This change is 

[emilio]

I have questions. Also, needs tests. Thanks!

[emilio]

Wouldn't this drop the vector afterwards, causing a use after free the next time it's accessed? Also, I see nothing updating the ptr field, which may change I suppose.

[torkleyy]

Yes, it would be dropped afterwards. I think you should get the new pointer, len and capacity from the vec and mem::forget it afterwards.

[emilio]

If the item has a destructor, what calls it?

[Xaeroxe]

@emilio I've addressed your comments and added tests, thanks for the help!

[Xaeroxe]

Travis only failed on nightly. Probably a regression in nightly.

EDIT: woops nevermind, looks like I accidentally broke nostd support. I just added a commit that should fix this.

[emilio]

Looks fine to me. Can you please squash the commits?

Maybe @mbrubeck or @jdm want to take another look at this, too.

Thanks!

[emilio]

nit: This can just be self.len -= del;, since del cannot be less than zero.

[Xaeroxe]

Squished and addressed your nit.

[Xaeroxe]

r? @mbrubeck and @jdm

[emilio]

nit: no need for the if, that's what I wanted to avoid in the first place :)

[emilio]

nit: Can't we just do self.swap(i - del, i)? We implement DerefMut<[A::Item]>.

[emilio]

Indeed if we do that and avoid the ptr::read, calling truncate() at the end, we can write this just with safe code, right?

[Xaeroxe]

@emilio Thanks for the review! The code has been greatly simplified and is no longer unsafe.

[jdm]

@bors-servo: r=emilio,jdm
This is much easier to reason about now; thanks!

[bors-servo]

📌 Commit 5879b5e has been approved by emilio,jdm

[bors-servo]

⌛ Testing commit 5879b5e with merge 6cbfbe2...

[bors-servo]

☀️ Test successful - status-travis
Approved by: emilio,jdm
Pushing 6cbfbe2 to master...

[Xaeroxe]

More out of curiosity than any kind of dire need, does this project have any kind of regular release cycle?

[jdm]

It is released whenever someone makes a PR that updated the Cargo.toml version.

[mbrubeck]

This didn't occur to me until just now, but we might want to change the retain signature to pass &mut A::Item to the callback instead of &A::Item. The Rust library team wishes they could make Vec::retain pass &mut T, and has used &mut for newer methods like HashMap::retain:

rust-lang/rust#39560 (comment)

[Xaeroxe]

Why would you want to mutate in a retain? It seems like you'd be better off doing

for thing in things {
     // Mutate thing
}
things.retain(/*closure*/);

I like this approach better as it's more clear that there is a side effect happening.

[mbrubeck]

for thing in things {
     // Mutate thing
}
things.retain(/*closure*/);

I like this approach better as it's more clear that there is a side effect happening.

Putting the mutation into a separate loop might require allocating additional memory to save intermediate state, while just using retain can use the results immediately. For example, if you want to send a message to a bunch of MPSC channels, and discard any senders whose receivers have hung up:

channels.retain(|sender| sender.send(message).is_ok());

This can't be done in two separate loops like the code above, without saving additional state.

Even in cases that can be easily separated, iterating over the collection twice instead can be a significant performance penalty. And since the retain function does have unique (&mut T) access to the collection, there's no reason for it to give out only shared (&T) references.

[Xaeroxe]

Alright that all makes sense. The current retain got released in 0.4.3 which still has 0 downloads so if we hurry and yank it we can still fix this.

[mbrubeck]

It might be less hassle to just fix this next time we make a release with breaking changes.