search

Lokathor / tinyvec

Just, really the littlest Vec you could need. So smol.
https://docs.rs/tinyvec
Apache License 2.0
648 stars 49 forks source link

Add retain_mut #198

Closed Fuuzetsu closed 3 months ago

Fuuzetsu commented 3 months ago

This adds a retain_mut method to ArrayVec and TinyVec that's just retain but with mutable references. This method is present on std::vec::Vec as of a while ago so it's nice to have in tinyvec too.

Fixes #195

Fuuzetsu commented 3 months ago

The implementation is just copy-pasted retain with a change to pass in mutable reference in.

Fuuzetsu commented 3 months ago

I guess this fails on 1.47 because retain_mut on std::vec::Vec wasn't present yet. I'm not sure how to deal with it.

The options are:

  1. bump MSRV
  2. do not add retain_mut
  3. Use something like https://crates.io/crates/rustversion/ to only add retain_mut for rustc versions where retain_mut is present in the stdlib.

Seems Vec::retain_mut appeared in 1.61.0 according to https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1610-2022-05-19 (2 years ago!)

I guess I'd lean towards the third option?

Fuuzetsu commented 3 months ago

I had a look at Cargo.toml, seems there's:

# features that require rustc 1.57
# add try_reserve functions to types that heap allocate.
rustc_1_57 = ["rustc_1_55"]

So perhaps we add rustc_1_61 feature?

Fuuzetsu commented 3 months ago

I guess 4th option is to only add retain_mut to ArrayVec as that's manually implemented and not add it to TinyVec but that's sad.

Lokathor commented 3 months ago

Adding rustc_1_61 as a new feature seems best. It's very easy for users to understand.

Fuuzetsu commented 3 months ago

@Lokathor Thanks, I added a commit that adds the feature. Not sure if I have to change something else, like docs features etc. Let me know if I have to change something.

Lokathor commented 3 months ago

This seems to pass CI now at least.

Can you add some test cases for this new method? The TinyVec method just calls the version in ArrayVec or Vec, but the new ArrayVec code should have some test cases.

Fuuzetsu commented 3 months ago

This seems to pass CI now at least.

Can you add some test cases for this new method? The TinyVec method just calls the version in ArrayVec or Vec, but the new ArrayVec code should have some test cases.

I've added some tests. Technically it was tested already as much as retain: via doctest on ArrayVec::retain_mut...; but sure, I added a few tests that demonstrate some obvious cases. Not sure if you're after some specific tests. I wonder if std has some tests for Vec::retain_mut, maybe we should copy-paste those in because semantics should be pretty much the same.

Lokathor commented 3 months ago

released in tinyvec-1.7.0