Add From<[Elem; N]> for Packed Array and Optimize From<Vec<Elem>>

[Dheatly23]

When making From array impl, i realized it and Vec are very similiar in semantics.

From array is useful to create small PackedArray quickly.

No From<Box<[Elem]>> since boxed slice can be converted into Vec as intermediate.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-827

[Bromeon]

Thanks a lot!

Not yet very sure about the safety invariant, added some comments. Do we really gain performance with it?

[Bromeon]

The // SAFETY assertion is not just a regular comment that explains what happens, but it needs to reason why and how the safety assumptions (specified in the function docs) are upheld.

[Bromeon]

Would also be good to have an empty line before it.

[Bromeon]

This is unnecessarily hard to understand, could you just write a

if vec.is_empty() {
    return Self::new();
}

instead?

[Dheatly23]

So something like this?

if vec.is_empty() {
    return Self::new();
}
let len = vec.len();
let mut ret = Self::default_with_size(len);

I don't know, but it doesn't save enough space to justify default_with_size and stand out with above Froms.

[Bromeon]

Yeah, maybe I was too optimistic with the code reuse here... Otherwise it's OK to not have default_with_size. More important than a bit of duplication or fewer lines of code is readability (which is problematic with the match version).

[Bromeon]

It's a bit annoying that Godot doesn't have a "resize without default-construct" (à la Vec::set_len), nor a "reserve" function.

But I don't see a way around default-constructing each element and then dropping it. "Dropping" is probably simplest done via = moves (which calls drop on lhs operand). Even if you want to use copy_nonoverlapping, you need to individually call drop_in_place() on each default-constructed element beforehand.

The alternative is appending elements to an empty collection (e.g. involving Vec::drain), which may be more expensive due to reallocations. Would need measurements to be sure.

[Bromeon]

Some more comments, thanks for the update!

Is there a concrete use case for supporting array conversions? People are always quick to add From with arrays, but in reality very few people use actual arrays, it's almost always Vec due to the dynamic nature. The syntax you mentioned is already possible with slices (although it can be tiny bit more expensive due to copy):

PackedInt32Array::from(&[1, 2])

Not against it, just wondering 🙂

[Bromeon]

Could you add an integration test for From<Vec>? Maybe with a packed array type that isn't yet used, e.g. Color...

[Bromeon]

Suggested change

	/// Moves elements from slice.
	/// Moves elements from slice.

Comments should express more than the function name, or be omitted.

Here, you could mention something like

Suggested change

	/// Moves elements from slice.
	/// Drops all elements in self and replaces them with ones in provided pointer range.

[Bromeon]

Thanks a lot for the continuous improvements!