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

Suggested API change – extend/collect should panic when capacity overflows #163

Closed
whatisaphone opened this issue Jul 24, 2020 · 4 comments · Fixed by #175
Closed

Suggested API change – extend/collect should panic when capacity overflows #163

whatisaphone opened this issue Jul 24, 2020 · 4 comments · Fixed by #175

Comments

@whatisaphone
Copy link

I noticed an API inconsistency. ArrayVec::push panics if you overflow the capacity, but ArrayVec::extend silently drops elements if you overflow the capacity.

I prefer the panicking behavior, since otherwise you run the risk of accidentally losing iterator elements. imo that's not an intuitive failure mode. I'd suggest changing extend to panic on overflow, and introduce a fallible try_extend (after all, you already have try_extend_from_slice), try_from_iterator, and probably try_collect and a trait to go with it.

This is a breaking change. The migration path would be pretty straightforward:

// previous version
arrayvec.extend(iterator);

// next version
arrayvec.extend(iterator.take(arrayvec.remaining_capacity());
// -- OR --
let _ignore_result = arrayvec.try_extend(iterator);
@whatisaphone whatisaphone changed the title Suggested API change Suggested API change – extend/collect should panic when capacity overflows Jul 24, 2020
@bluss
Copy link
Owner

bluss commented Oct 23, 2020

I'm inclined to agree, but would like to hear other's thoughts as well. It's not great that extend/fromiterator may panic, but there are only these two (bad) options.

@tbu-
Copy link
Collaborator

tbu- commented Jan 6, 2021

I'm also in favor of panicking.

@bluss
Copy link
Owner

bluss commented Jan 6, 2021

I think this change could be done for the next breaking version update - looking like the const generics update

@bluss
Copy link
Owner

bluss commented Mar 23, 2021

The behaviour change is implemented in #175, but .try_extend() is not implemented yet (that's a new feature, not a behaviour change). A new issue is needed for that.

I'm a bit unsure about the try_extend name because I'm not so keen to have a "try" method that can return an error, but still have an effect in the error case. Since it's an iterator we wouldn't know its length until we iterate it. So maybe it could be called something else than "try_extend". Compare with try_extend_from_slice: it doesn't partly extend in the error case - it leaves the arrayvec is unchanged on error.

@bluss bluss mentioned this issue Mar 23, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants