Implement packed arrays non-generically #91
Conversation
ttencate
force-pushed
the
feature/packed_array
branch
from
a7a0f00
to
8ac456c
Compare
| let mut array = Self::new(); | ||
| let len = slice.len(); | ||
| if len == 0 { | ||
| return array; | ||
| } | ||
| array.resize(len); | ||
| let ptr = array.ptr_mut(0); | ||
| for (i, element) in slice.iter().enumerate() { | ||
| // SAFETY: The array contains exactly `len` elements, stored contiguously in memory. | ||
| unsafe { | ||
| // `GodotString` does not implement `Copy` so we have to call `.clone()` | ||
| // here. | ||
| *ptr.offset(to_isize(i)) = element.clone(); | ||
| } | ||
| } | ||
| array |
There was a problem hiding this comment.
Could this not be moved to FromIterator and then reused from here?
There, instead of slice.len() we could use size_hint(), I'm almost certain that provides an accurate number for slices.
There was a problem hiding this comment.
It does for slices, but not for iterators in general. So we would need a lot more complexity to deal with iterators that don't have a size hint, whose size hint is incorrect, or even whose size hint changes during iteration. Let's keep it simple and stupid for now.
| self.check_bounds(index); | ||
| // SAFETY: We just checked that the index is not out of bounds. | ||
| let ptr = unsafe { | ||
| (interface_fn!($operator_index_const))(self.sys(), to_i64(index)) as *const $Element |
There was a problem hiding this comment.
Maybe also here, split statements into FFI call and pointer cast.
Explicit type annotations for the pointer is currently not possible, but can easily be added once we have generics.
There was a problem hiding this comment.
The type is either $Element or TagType or TagString. Had to add another macro argument for this, and it'll couple the implementation more tightly to the generated code. Which, I suppose, is exactly the idea :)
| assert!( | ||
| index < len, | ||
| "Array index {} is out of bounds: length is {}", | ||
| index, | ||
| len | ||
| ); |
There was a problem hiding this comment.
Suggestion:
assert!(
index < len,
"Array index {index} is out of bounds: length is {len}"
);Does clippy not complain here? I just fixed a ton of similar issues 😀
A few other places like this...
There was a problem hiding this comment.
Not in CI at least... maybe it needs a Rust version upgrade?
There was a problem hiding this comment.
I'm on 1.66.0 stable and not getting complaints when I run cargo clippy in the project root 🤷 I'll fix these ones but we might need a more structural solution here.
There was a problem hiding this comment.
I encountered them e.g. in this CI run, but it was format! in that case.
The Clippy lint doc does not explicitly mention assert!, but clicking on Related issues shows some issues mentioning assert!. Hmm... 🤔
tryBuild succeeded: |
ttencate
force-pushed
the
feature/packed_array
branch
from
8ac456c
to
2ef6959
Compare
|
Small conflicts introduced by just-merged #90, other than that it looks good to me! |
The ideal is to have a `struct PackedArray<T>`, where `T` implements some `PackedArrayElement` trait. But since the autogenerated `InnerPacked*Array` types are entirely independent at the moment and don't implement a common trait, that would require indirecting every method implementation through `PackedArrayElement`, which results in mentioning each method call three times: once in `impl PackedArray<T>`, once in `PackedArrayElement` and once in `impl PackedArrayElement for T`. So, for now, we use a big (but quite straightforward) macro to define each `Packed*Array` as a separate type.
ttencate
force-pushed
the
feature/packed_array
branch
from
2ef6959
to
82a8a16
Compare
|
bors r+ |
|
Build succeeded: |
The ideal is to have a
struct PackedArray<T>, whereTimplements somePackedArrayElementtrait. But since the autogeneratedInnerPacked*Arraytypes are entirely independent at the moment and don't implement a common trait, that would require indirecting every method implementation throughPackedArrayElement, which results in mentioning each method call three times: once inimpl PackedArray<T>, once inPackedArrayElementand once inimpl PackedArrayElement for T.So, for now, we use a big (but quite straightforward) macro to define each
Packed*Arrayas a separate type.