Prepare 0.4, more vec-like api and error reform #61

bluss · 2017-07-30T11:21:21Z

`push, insert` etc

With the goal of making arrayvec more of a "drop-in" for Vec, we realign the signatures methods .push(elt), .insert(index, elt) and .remove(index).

pub fn push(&mut self, element: A::Item);
pub fn insert(&mut self, index: usize, element: A::Item);
pub fn remove(&mut self, index: usize) -> A::Item;
pub fn swap_remove(&mut self, index: usize) -> A::Item;

`try_push` etc

Mimicing something like fallible allocation, we have the fallible new methods:

pub fn try_push(&mut self, element: A::Item) -> Result<(), CapacityError<A::Item>>;
pub fn try_insert(&mut self, index: usize, element: A::Item) -> Result<(), CapacityError<A::Item>>;

Notice that try_insert still panics if index is out of bounds.

`pop_at` etc

Mimicing .pop(), we have new “checked” versions of remove, swap_remove:

pub fn pop_at(&mut self, index: usize) -> Option<A::Item>;
pub fn swap_pop(&mut self, index: usize) -> Option<A::Item>;

The rationale is: indexing out of bounds is not a regular runtime error, it's normally a contract violation. So when we introduce checked versions of such API, then we get “checked” versions that return Option. Non-presence is not necessarily an error.

`ArrayString::push` etc

ArrayString: push, push_str now have the same signature as corresponding on String. New methods try_push, try_push_str.

pub fn push(&mut self, c: char);
pub fn try_push(&mut self, c: char) -> Result<(), CapacityError<char>>;
pub fn push_str(&mut self, s: &str);
pub fn try_push_str<'a>(&mut self, s: &'a str) -> Result<(), CapacityError<&'a str>>;

Closes #46
Closes #45

Miscellaneous Changes

odds is no longer a public dependency. We want the flexibility of dropping/upgrading this dep at will
Rust version changes

- Add try_insert / try_remove to be the old fallible variants that return errors - Insert that pushes out if full does no longer exist -- full vec is an error

This means odds is no longer a public dependency, which simplifies the version story for arrayvec

Also use pub(crate), requiring Rust 1.18

…llible

Required rust version should be in rustdoc Reason: Written in one or a few places. Information is versioned with the crate on docs.rs

Use same signatures (meaning: panics on errors). Add fallible versions .try_push() and .try_push_str()

tbu- · 2017-07-30T21:04:45Z

I'd prefer methods that panic on invalid indices and return an error on capacity error. Maybe we could also ask other developers about this. I think the proposed fallible allocation API on internals.rust-lang.org also behaves that way.

bluss · 2017-07-30T21:33:22Z

Thanks for the feedback. Even if this crate is moving at glacial pace.

From my #55 (comment)

Comaintainers / maintainers that are interested in taking care of this crate? I'm not planning on stepping down, but I clearly don't have enough time to do the whole job.

tbu- · 2017-07-30T22:16:53Z

I'd be interested in taking care of this crate. What do you think?

bluss · 2017-07-31T17:58:17Z

Sure. We can do it together? I'm not sure what you have in mind. In one way we want to guide this crate towards 1.0, but we're missing a few pieces from Rust for that (ManuallyDrop in Rust 1.20 and uninit data in inline field clarification is ??). With API overhaul it feels like we are ready with every we can do on our side.

(Clearly integer generics is not something to wait for 1.0 for)

tbu- · 2017-08-01T23:22:35Z

Yes, of course together.

I would like to discuss the error returning some more (this defnitely won't a roadblocker for contributing either way). I'd also like to finally introduce ArrayVecCopy, one way or the other, this is definitely another thing we could do without outside updates.

bluss · 2017-08-02T09:43:27Z

Sure, that sounds good!

I had some discussion on #rust-libs and they sort of brought me over to mixing panic and result for insert.

If we think of arrayvec as a Vec with a particular inline allocator, we have the problem of fallible allocation (capacity is limited); and a hypothetical fallible allocation Vec::try_insert would panic on out of bounds, but return an error for allocator errors.

It's not obvious that fallbility of insert is just about allocation/capacity, but with the comparison to Vec it seems ideal to make it that way.

And thus another reminder that arrayvec is obsolete once custom allocators get so far that Vec can be backed by an array! That sounds cool.

bluss · 2017-08-02T09:51:25Z

Sigh, I don't know anymore if it's best to have the error cases in push or in the separate fallible version. Both those seem fine

tbu- · 2017-08-03T23:14:07Z

I'd prefer the non-prefixed ones, but that's no strong opinion.

Do you want to finish this API change yourself or should I do it?

bluss · 2017-08-03T23:50:56Z

I can finish it

try_insert has a capacity error, but panics if the index is out of bounds.

We turn these into "checked" removals, similar to `.pop()`. Name swap_pop seems straightforward and nice, remove_opt is not as certain.

bluss · 2017-08-05T16:05:35Z

ping @tbu- if you want. This is getting better with the feedback IMO. Most of your thoughts have been incorporated. PR description updated.

tbu-

Seeing this functions that just do function().unwrap() makes me a bit uneasy, but I guess it's ok.

All in all, it looks good to me.

tbu- · 2017-08-05T20:10:57Z

src/lib.rs

+        panic!(concat!("ArrayVec::", $method_name, ": index {} is out of bounds in vector of length {}"),
+               $index, $len)
+    }
+}


In the standard library, this is in a separate method to remove code size from the hot code path (where the array indices usually aren't out of bounds).

Right. The approach is to more or less go for correct with this PR and safe fast for later. I'm not sure we can find a difference, but it would be nice to at least put the panic formatting code in a place that is independent of the type parameters.

tbu- · 2017-08-05T21:29:45Z

src/lib.rs

+    /// The `index` must be strictly less than the length of the vector.
+    ///
+    /// ***Panics*** if the `index` is out of bounds.
+    /// vector.


Extra word.

bluss · 2017-08-05T22:38:30Z

Yes -- the unwraps have room for improvement. They should all be Result::unwrap's so they have their error information there in a basic form.

bluss · 2017-08-05T22:47:00Z

Let's go, so that we finally clear this hurdle.

bluss · 2017-08-05T22:48:05Z

New rust version requirement is open to suggestions... Right now I can't imagine that the current era Rust versions are going to break like Rust 1.0 - 1.10 ish seem to have done, but maybe I'll be surprised.

bluss added 12 commits July 30, 2017 12:57

FEAT: Change .insert() and .remove() to match Vec

5dcb5ab

- Add try_insert / try_remove to be the old fallible variants that return errors - Insert that pushes out if full does no longer exist -- full vec is an error

FEAT: Copy RangeArgument from crate odds

79ccdcd

This means odds is no longer a public dependency, which simplifies the version story for arrayvec

FEAT: Proper Error impls for errors

dadeedb

Also use pub(crate), requiring Rust 1.18

FEAT: Make .push() use panics for errors, add .try_push()

98af43c

FEAT: Make swap_remove return the element, add try_swap_remove for fa…

da62042

…llible

FEAT: Test in Travis from Rust 1.18

3a235d7

BUG: Fix serde code for push change

1d08021

DOC: Rust version in docs

a59b5c0

Required rust version should be in rustdoc Reason: Written in one or a few places. Information is versioned with the crate on docs.rs

MAINT: Use serde-1 feature in docs.rs

707ddc3

DOC: Update docs for debug assertions

4d78870

FEAT: Refactor errors. Insert is either out of bounds or capacity error

345dd33

FEAT: Align ArrayString .push and .push_str with String

1e83039

Use same signatures (meaning: panics on errors). Add fallible versions .try_push() and .try_push_str()

bluss added 3 commits August 5, 2017 17:40

FEAT: Remove InsertError again

bc9e036

try_insert has a capacity error, but panics if the index is out of bounds.

FEAT: Turn try_remove/try_swap_remove into remove_opt and swap_pop

c5e0b80

We turn these into "checked" removals, similar to `.pop()`. Name swap_pop seems straightforward and nice, remove_opt is not as certain.

MAINT: Rust version Rust 1.15

6ad4ca7

bluss force-pushed the vec-like-api branch from 77adf82 to 6ad4ca7 Compare August 5, 2017 16:04

bluss added 4 commits August 5, 2017 18:06

BUG: Make error module private again

27b8a0c

BUG: Fix import of CapacityError in tests

cf76a83

FEAT: Rename .remove_opt() to .pop_at()

313ebe8

DOC: Copyedit the docs a bit

4c90dc7

DOC: Use ArrayVec in doc for drain

689e55b

tbu- reviewed Aug 5, 2017

View reviewed changes

DOC: Fix typo in doc comment

696459d

bluss merged commit e894d00 into master Aug 5, 2017

bluss deleted the vec-like-api branch August 5, 2017 22:48

bluss mentioned this pull request Aug 5, 2017

arrayvec 1.0 #48

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare 0.4, more vec-like api and error reform #61

Prepare 0.4, more vec-like api and error reform #61

bluss commented Jul 30, 2017 •

edited

tbu- commented Jul 30, 2017

bluss commented Jul 30, 2017

tbu- commented Jul 30, 2017

bluss commented Jul 31, 2017

tbu- commented Aug 1, 2017

bluss commented Aug 2, 2017

bluss commented Aug 2, 2017

tbu- commented Aug 3, 2017

bluss commented Aug 3, 2017

bluss commented Aug 5, 2017

tbu- left a comment

tbu- Aug 5, 2017

bluss Aug 5, 2017

tbu- Aug 5, 2017

bluss Aug 5, 2017

bluss commented Aug 5, 2017

bluss commented Aug 5, 2017

bluss commented Aug 5, 2017

Prepare 0.4, more vec-like api and error reform #61

Prepare 0.4, more vec-like api and error reform #61

Conversation

bluss commented Jul 30, 2017 • edited

push, insert etc

try_push etc

pop_at etc

ArrayString::push etc

Miscellaneous Changes

tbu- commented Jul 30, 2017

bluss commented Jul 30, 2017

tbu- commented Jul 30, 2017

bluss commented Jul 31, 2017

tbu- commented Aug 1, 2017

bluss commented Aug 2, 2017

bluss commented Aug 2, 2017

tbu- commented Aug 3, 2017

bluss commented Aug 3, 2017

bluss commented Aug 5, 2017

tbu- left a comment

Choose a reason for hiding this comment

tbu- Aug 5, 2017

Choose a reason for hiding this comment

bluss Aug 5, 2017

Choose a reason for hiding this comment

tbu- Aug 5, 2017

Choose a reason for hiding this comment

bluss Aug 5, 2017

Choose a reason for hiding this comment

bluss commented Aug 5, 2017

bluss commented Aug 5, 2017

bluss commented Aug 5, 2017

bluss commented Jul 30, 2017 •

edited

`push, insert` etc

`try_push` etc

`pop_at` etc

`ArrayString::push` etc