-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
add slice::array_chunks
to std
#74373
Conversation
src/libcore/tests/slice.rs
Outdated
#[test] | ||
fn test_array_chunks_infer() { | ||
let v: &[i32] = &[0, 1, 2, 3, 4, -4]; | ||
let c = v.array_chunks(); | ||
for &[a, b, c] in c { | ||
assert_eq!(a + b + c, 3); | ||
} | ||
|
||
let v2: &[i32] = &[0, 1, 2, 3, 4, 5, 6]; | ||
let total = v2.array_chunks().map(|&[a, b]| a * b).sum::<i32>(); | ||
assert_eq!(total, 2 * 3 + 4 * 5); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at this beauty
Is there also a PR for |
Feel free to open one once |
Neat!! Just one little thought, should |
Presumably this fixes #60735. |
This looks good to me. Only question to me is the name. Too bad that this is just a strictly better version of the existing chunks_exact. array_chunks is good, but maybe something that starts with chunks_ to group it with the other chunk methods alphabetically (chunks_array doesn't sound amazing but I don't have an immediate better alternative). |
There are rare edge cases where The docs order is a valid concern I didn't previously consider.
We would end up with all const generics methods grouped together which IMO is also desirable. I fairly strongly prefer |
src/libcore/slice/mod.rs
Outdated
/// | ||
/// # Panics | ||
/// | ||
/// Panics if `N` is 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a note here that says that future versions of this are allowed to panic at compile-time if N
is 0 instead of run-time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While that's desirable, I am not sure how feasible it is to change this after stabilization.
Opened a discussion on zulip about this: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/array_chunks.20of.20size.200/near/204001667
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up changing array_chunks::<0>()
to a compile time error. While we may removing this bound before this method hits stable, it seems like the better default for now.
error[E0277]: the trait bound `[(); 0]: core::slice::sealed::NonZero` is not satisfied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, there's also an upper bound on array lengths for any non-ZST T
, but that should get a compile-time error: the type `...` is too big for the current architecture
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaah, nm. Adding this bound breaks type inference. Looks like we may have to live with a runtime error for now
I've just seen that in my codebase I can replace all windows with array_windows. |
Since this has the semantics of |
I personally do not feel this way as |
How about
? After all, these functions are based on Or we could use |
I'd prefer something else than |
|
That's true. 😂 Few more name suggestions along with others discussed so far, just for comparison, all at once:
vs
vs
vs
vs
Can't think of any other suffix 🤔 Hope the last one works for @lcnr and others. |
I personally still prefer At least for me personally |
Fair enough. After considering all alternative names I could think of, |
A slight variation could use the adjective --
That's exactly what this PR does! The closures like |
src/libcore/slice/mod.rs
Outdated
/// | ||
/// The chunks are slices and do not overlap. If `N` does not divide the length of the | ||
/// slice, then the last up to `N-1` elements will be omitted and can be retrieved | ||
/// from the `remainder` function of the iterator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we mention that this is equivalent to chunks
? Not specifying this might let people get the wrong idea, chunks_exact
. Maybe we should also have a version for chunks_exact
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually equivalent to chunks_exact
, as it returns &[T; N]
, which must always contain exactly N elements.
Because of this, implementing chunks
usings arrays/const generics doesn't really make sense here.
Added a note to the method docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, then how about a version for chunks
? If there is a generic version for chunks_exact
I think there should be one for chunks
too.
If we took chunks_array
then the chunks
version will be chunks_array_not_exact
? Or maybe we should keep the _exact
for this?
yeah. Thanks 😅 |
@bors r+ |
📌 Commit 1b90e78393866dbedc859bd09a353ce83112e475 has been approved by |
That submodule was noticed before, then came back with the tracking issues... @bors r- |
................what? sry fixed it @bors r=withoutboats |
📌 Commit d51b71a has been approved by |
☀️ Test successful - checks-actions, checks-azure |
Add `slice::array_chunks_mut` This follows `array_chunks` from rust-lang#74373 with a mutable version, `array_chunks_mut`. The implementation is identical apart from mutability. The new tests are adaptations of the `chunks_exact_mut` tests, plus an inference test like the one for `array_chunks`. I reused the unstable feature `array_chunks` and tracking issue rust-lang#74985, but I can separate that if desired. r? `@withoutboats` cc `@lcnr`
Implement split_array and split_array_mut This implements `[T]::split_array::<const N>() -> (&[T; N], &[T])` and `[T; N]::split_array::<const M>() -> (&[T; M], &[T])` and their mutable equivalents. These are another few “missing” array implementations now that const generics are a thing, similar to rust-lang#74373, rust-lang#75026, etc. Fixes rust-lang#74674. This implements `[T; N]::split_array` returning an array and a slice. Ultimately, this is probably not what we want, we would want the second return value to be an array of length N-M, which will likely be possible with future const generics enhancements. We need to implement the array method now though, to immediately shadow the slice method. This way, when the slice methods get stabilized, calling them on an array will not be automatic through coercion, so we won't have trouble stabilizing the array methods later (cf. into_iter debacle). An unchecked version of `[T]::split_array` could also be added as in rust-lang#76014. This would not be needed for `[T; N]::split_array` as that can be compile-time checked. Edit: actually, since split_at_unchecked is internal-only it could be changed to be split_array-only.
Implement split_array and split_array_mut This implements `[T]::split_array::<const N>() -> (&[T; N], &[T])` and `[T; N]::split_array::<const M>() -> (&[T; M], &[T])` and their mutable equivalents. These are another few “missing” array implementations now that const generics are a thing, similar to rust-lang#74373, rust-lang#75026, etc. Fixes rust-lang#74674. This implements `[T; N]::split_array` returning an array and a slice. Ultimately, this is probably not what we want, we would want the second return value to be an array of length N-M, which will likely be possible with future const generics enhancements. We need to implement the array method now though, to immediately shadow the slice method. This way, when the slice methods get stabilized, calling them on an array will not be automatic through coercion, so we won't have trouble stabilizing the array methods later (cf. into_iter debacle). An unchecked version of `[T]::split_array` could also be added as in rust-lang#76014. This would not be needed for `[T; N]::split_array` as that can be compile-time checked. Edit: actually, since split_at_unchecked is internal-only it could be changed to be split_array-only.
Implement split_array and split_array_mut This implements `[T]::split_array::<const N>() -> (&[T; N], &[T])` and `[T; N]::split_array::<const M>() -> (&[T; M], &[T])` and their mutable equivalents. These are another few “missing” array implementations now that const generics are a thing, similar to rust-lang#74373, rust-lang#75026, etc. Fixes rust-lang#74674. This implements `[T; N]::split_array` returning an array and a slice. Ultimately, this is probably not what we want, we would want the second return value to be an array of length N-M, which will likely be possible with future const generics enhancements. We need to implement the array method now though, to immediately shadow the slice method. This way, when the slice methods get stabilized, calling them on an array will not be automatic through coercion, so we won't have trouble stabilizing the array methods later (cf. into_iter debacle). An unchecked version of `[T]::split_array` could also be added as in rust-lang#76014. This would not be needed for `[T; N]::split_array` as that can be compile-time checked. Edit: actually, since split_at_unchecked is internal-only it could be changed to be split_array-only.
Now that #74113 has landed, these methods are suddenly usable. A rebirth of #72334
Tests are directly copied from
chunks_exact
and some additional tests for type inference.r? @withoutboats as you are both part of t-libs and working on const generics. closes #60735