tokio-rs/slab

Remove should return `Option<T>`

jonhoo opened this issue · 5 comments

jonhoo commented

Currently, remove(key) returns a T and panics if key is not in the Slab. This matches the API of Vec::remove. However, Slab is more like a Map than it is a Vec, and should probably do the same as HashMap::remove and BTreeMap::remove, which return Option<T> (and thus do not panic).

carllerche commented

Yes, but that would be breaking :(

jonhoo commented

True. Maybe for next major version? It's a pretty straigthforward change for users.

Alternatively, or in the meantime, we could provide something like VecMap::take(key) -> Option<T>. Has a nice analogue to Option::take too.

tormol commented

I think remove() should stay as it is:
I expect that in most cases trying to remove a non-existing key means there is a bug, so if remove() returned Option people would just call .unwrap() afterwards. That is less ergonomic and will also result in a less descriptive panic message when there is a bug.

When one wants to handle the key not being present, calling contains() first is simple:

if slab.contains(key) {
    Some(slab.remove(key))
} else {
    None
}

Returning an Option makes sense for HashMap and BTreeMap because looking up a key is a nontrivial operation there, which makes calling contains() first inefficient.
For Slab on the other hand, all contains() does is comparing the length and then loading and checking the discriminant. I also assume the compiler will inline the functions and merge the checks in the code above.

  • 👍1
carllerche commented

Ok, so I will close (based on @jonhoo's 👍 )

Cheers.

timothee-haudebourg commented

I'm not quite convinced by @tormol argument since it could also be applied to the .get() function. I think providing an api that avoid panicking is always better. I don't see the lack of a proper error message as such a big issue, and the resulting api would be harmonized with the standard library.

  • 👍1