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

Confusion with push and insert methods #41

Open
Luro02 opened this issue Apr 18, 2020 · 1 comment
Open

Confusion with push and insert methods #41

Luro02 opened this issue Apr 18, 2020 · 1 comment

Comments

@Luro02
Copy link

Luro02 commented Apr 18, 2020

There seem to be two ways to add an element to a StableVec:

  1. StableVec::push, which inserts an element in the next free slot? It is not entirely clear by the documentation what this is supposed to be?

  2. StableVec::insert, which inserts an element at the specified position, but does panic if the index is out of bounds (which makes it quite inconvenient, because you would have to always call StableVec::reserve_for(index) and then call StableVec::insert, to prevent panics).

I am a bit confused about the behavior of those two methods.

  • I expect a function called push to push an element to the back of the vec (like Vec::push)
  • and I do not expect insert to panic if the index is out of bounds (I see this method more like a HashMap::<usize, T>::insert)

I would suggest adding the following methods:

  • StableVec::push_back(&mut self, value: T), appends an element to the back of the StableVec, ignoring free slots
  • StableVec::pop_back(&mut self) -> Option<T>, removes the last element from a StableVec

And changing the StableVec::insert method to not panic if the capacity is exhausted and instead resize the vector.

@LukasKalbertodt
Copy link
Owner

Regarding push:

next_push_index(): the index of the first slot (i.e. with the smallest index) that was never filled.

So push does indeed push to the end of the vector... in a sense. It certainly doesn't insert in the first free slot! next_push_index is a bit like Vec::len, so I think push already does exactly what you want.

pop_back already exists as remove_last.


Regarding insert: having insert allocate on its own might be a good idea. Note though that this crate is deliberately low level and flexibility and performance are more important than convenience. That said, panicking in insert still requires the out of bounds check. So I think there should be an unsafe method to insert without any bounds check.

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

No branches or pull requests

2 participants