Skip to content

[ER] Three Vec functions #82336

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

Closed
leonardo-m opened this issue Feb 20, 2021 · 4 comments
Closed

[ER] Three Vec functions #82336

leonardo-m opened this issue Feb 20, 2021 · 4 comments

Comments

@leonardo-m
Copy link

Some member functions that could be added to Vec:

This is similar to Vec::remove but like Vec::pop it returns an Option, avoiding panics:

pub fn Vec::pop_index(&mut self, index: usize) -> Option<T>;

(In alternative Vec::pop_front could often suffice).


Probably this was already suggested elsewhere:

pub unsafe fn get_debug<I>(&self, index: I) -> &<I as SliceIndex<[T]>>::Output;
pub unsafe fn get_debug_mut<I>(&mut self, index: I) -> &mut <I as SliceIndex<[T]>>::Output;

They contain a debug_assert!() to test the bounds only in debug builds. I think such debug_assert can't be added to get_unchecked/get_unchecked_mut because the semantics is different. They are unsafe, but they add a bit of safety during debug runs.

@jyn514
Copy link
Member

jyn514 commented Feb 20, 2021

@leonardo-m this can't work because the standard library isn't recompiled between debug and release builds, it's always compiled in release mode. Adding a debug_assert! to get_unchecked seems fine but it won't affect anything other than testing the standard library itself.

@the8472
Copy link
Member

the8472 commented Feb 20, 2021

(In alternative Vec::pop_front could often suffice).

I think Vec doesn't implement it because it would be a performance footgun as it has to move the entire Vec on each invocation, i.e. it's O(n), it's better to use VecDeque in such a case where pop_front is O(1).

@leonardo-m
Copy link
Author

I see, thank for the answers.
Regarding Vec::pop_index, it's not slower than Vec::remove, the difference is just in the API, it returns an Option...

@jyn514
Copy link
Member

jyn514 commented Feb 20, 2021

Regarding Vec::pop_index, it's not slower than Vec::remove, the difference is just in the API, it returns an Option...

This was discussed and rejected in #77480. Some relevant comments:

I feel like this is a convention change to Vec, and thus needs more than just a PR.
Because, as timvermeulen mentioned, why is this just to remove? If we have try_remove, why not also try_drain and try_split_off and try_splice and try_swap_remove and try_insert? (Not to mention slice methods, like try_split_at or try_rotate_left or ... And should VecDeque get all these methods too?)
The preconditions for the indexes on all these are simple enough to check yourself, so I don't think it's worth the API bloat to have non-panicking versions of all of them, and I don't think that any of them are special enough to have such a version when the others don't.

So pop_index would need an RFC, and the others are not possible to write.

@jyn514 jyn514 closed this as completed Feb 20, 2021
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

3 participants