Add array_chunks #1023
Add array_chunks #1023
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1023 +/- ##
==========================================
- Coverage 94.38% 94.32% -0.07%
==========================================
Files 48 51 +3
Lines 6665 6963 +298
==========================================
+ Hits 6291 6568 +277
- Misses 374 395 +21 ☔ View full report in Codecov by Sentry. |
Iterator::array_chunks exists on nightly and it's better to avoid the name collision especially if and when that's stabilized
|
Re the MSRV failure, I'm not sure what the idiomatic way is to trigger a PME on 1.63 based on |
You can do it like this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=233fb2f6ca3df52d15e3df96b8c4cb61 |
src/arrays.rs
Outdated
| /// Effectively assert!(N > 0) post-monomorphization | ||
| fn assert_positive<const N: usize>() { | ||
| trait StaticAssert<const N: usize> { | ||
| const ASSERT: bool; | ||
| } | ||
|
|
||
| impl<const N: usize> StaticAssert<N> for () { | ||
| const ASSERT: bool = { | ||
| assert!(N > 0); | ||
| true | ||
| }; | ||
| } | ||
|
|
||
| assert!(<() as StaticAssert<N>>::ASSERT); | ||
| } |
There was a problem hiding this comment.
Rust doesn't provide stacktraces for monomorphization errors, so folks will only see that an error occured in assert_positive — and not the code that actually required the assertion.
Define this as a macro and call that instead.
There was a problem hiding this comment.
Is the change in db21b2f what you meant?
| /// assert_eq!(Some([]), it.next()); | ||
| /// ``` | ||
| #[cfg(feature = "use_alloc")] | ||
| fn arrays<const N: usize>(self) -> Arrays<Self, N> |
There was a problem hiding this comment.
IMO array_chunks is the better name, since it differentiates it from array_windows.
There was a problem hiding this comment.
To add context, I was trying to avoid triggering the unstable-name-collisions lint, because of rust-lang/rust#100450. Should I change it back to array_chunks?
| pub struct Arrays<I: Iterator, const N: usize> { | ||
| iter: I, | ||
| partial: Vec<I::Item>, | ||
| } |
There was a problem hiding this comment.
It strikes me as a code smell that N doesn't appear in the definition here. Shouldn't partial be an ArrayBuilder<T, N>?
There was a problem hiding this comment.
Making it ArrayBuilder<T, N> a priori makes sense, but I had trouble with the bounds for the impl Clone in that case. Note that MaybeUninit<T>: Clone requires T: Copy.
Maybe it could also be array::IntoIter<T, N> or a hypothetical ArrayBuilder::IntoIter<T, N> (this is close to what the unstable Iterator::ArrayChunks does) but I think the unsafe code for that would need to be more careful.
The state you need to keep track of doesn't really depend on N (except partial.len() <= N but this is sort of incidental). However Arrays needs to have N as a parameter for Arrays::Item to be well-defined.
Suggestions?
Added an implementation of
arraysbased on the work innext_array(). Currently theN = 0produces a post-monomorphization error, see the discussion in #1012.Some other choices that I'm not sure about:
Nthen there is exactly one allocation the first timenext()returnsNone. I don't think this is a performance issue but this means the method has to be gated behinduse_alloc. The alternative would be something like implementingIntoIteratorforArrayBuilder, which seems possible but the safety invariants get more complicated. A middle ground would be to only gate theremainder()method, but this seems worse.ArrayChunks::remainder()return a custom type, so that it can beExactSizeIterator?N.