add `slice::array_chunks` to std by lcnr · Pull Request #74373 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation59 Commits7 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

jonas-schievink, darksv, DutchGhost, jplatte, rrbutani, pitdicker, leonardo-m, vincent-herlemont, 95th, shirshak55, and 25 more reacted with heart emoji

lcnr mentioned this pull request

Jul 15, 2020

Comment on lines 476 to 487

#[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]
assert_eq!(total, 2 * 3 + 4 * 5);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at this beauty

jplatte, eddyb, darksv, jyn514, withoutboats, stearnsc, K900, rrbutani, rochacbruno, quat1024, and 38 more reacted with heart emoji

Is there also a PR for array_windows? :)

Feel free to open one once T-libs decides this gets merged.
There are too many methods for me to do this all in one go 😄

Neat!! Just one little thought, should .array_chunks_mut::<N>() also be included in this PR?

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 chunks_exact is useful (when dealing with variably sized chunks) 😅

The docs order is a valid concern I didn't previously consider.
However, once we add the remaining const generic methods:

array_chunks_mut
array_windows
array_rchunks
array_rchunks_mut

We would end up with all const generics methods grouped together which IMO is also desirable.

I fairly strongly prefer array_chunks over chunks_array and wasn't yet able to come up with a good alternative starting with chunks_. chunks_const doesn't sound completely horrible, but I still prefer array_chunks here.

///
/// # Panics
///
/// Panics if `N` is 0.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

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 chunks_exact and not those of chunks, I feel this should be called array_chunks_exact (even though that’s lengthy and the corresponding array_chunks with chunks semantics is impossible).

Since this has the semantics of chunks_exact and not those of chunks, I feel this should be called array_chunks_exact

I personally do not feel this way as array_ very strongly implies fixed length. Anything which is not exact would
not be able to use an array, so using exact here is therefore unnecessarily redundant in my view.

@lcnr

The docs order is a valid concern I didn't previously consider.
However, once we add the remaining const generic methods:

array_chunks_mut

array_windows

array_rchunks

array_rchunks_mut

We would end up with all const generics methods grouped together which IMO is also desirable.

I fairly strongly prefer array_chunks over chunks_array and wasn't yet able to come up with a good alternative starting with chunks_. chunks_const doesn't sound completely horrible, but I still prefer array_chunks here.

How about

chunks_const
chunks_mut_const
windows_const
rchunks_const
rchunks_mut_const

After all, these functions are based on const generics and thus take a const N as generic argument. I believe this also addresses @SimonSapin's concern (to use _exact in its name), as _const implies that anyway — that too, more strongly!

Or we could use _fixed suffix instead. I'd prefer the _const though. 😄

@lcnr

The docs order is a valid concern I didn't previously consider.
However, once we add the remaining const generic methods:

array_chunks_mut

array_windows

array_rchunks

array_rchunks_mut

We would end up with all const generics methods grouped together which IMO is also desirable.
I fairly strongly prefer array_chunks over chunks_array and wasn't yet able to come up with a good alternative starting with chunks_. chunks_const doesn't sound completely horrible, but I still prefer array_chunks here.

How about
* `chunks_const`

* `chunks_mut_const`

* `windows_const`

* `rchunks_const`

* `rchunks_mut_const`
?

After all, these functions are based on const generics and thus take a const N as generic argument. I believe this also addresses @SimonSapin's concern (to use _exact in its name), as _const implies that anyway — that too, more strongly!

Or we could use _fixed suffix instead. I'd prefer the _const though. smile

I'd prefer something else than _const, as it sort of almost suggest that the function is a const fn and that the resulting Iterator is const fn compatible

mut_const is too much of a contradiction, even though those qualifiers are referring to different things.

mut_const is too much of a contradiction, even though those qualifiers are referring to different things.

That's true. 😂

Few more name suggestions along with others discussed so far, just for comparison, all at once:

array_chunks (original; will not group with the old members)
array_chunks_mut

chunks_array (flipped; but does not look good)
chunks_mut_array

chunks_const
chunks_mut_const (looks contradictory)

chunks_fixed (does not look good enough to me)
chunks_mut_fixed

chunks_static (as the size is known statically)
chunks_mut_static

Can't think of any other suffix 🤔

Hope the last one works for @lcnr and others.

I personally still prefer array_chunks here as I somewhat elaborated on in #74373 (comment).

At least for me personally static is too strongly related to lifetimes to also consider it to describe sizes.
I do think chunks_fixed seems okay, so I would also be fine with that.

Fair enough.

After considering all alternative names I could think of, array_chunks seems pretty good to me now, also from another perspective: it could return &[T; N] instead of &[T]. And since each element (chunk) is an array, it could be named as chunks_as_array — looks lengthy though, but it (or simply chunks_array) has one advantage: grouping all chunks functions.

A slight variation could use the adjective -- arrayed_chunks or chunks_arrayed.

it could return &[T; N] instead of &[T].

That's exactly what this PR does! The closures like |[a, b]| would have refutable patterns if they were slices.

///
/// 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.

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?

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

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?

lcnr mentioned this pull request

Jul 20, 2020

lcnr mentioned this pull request

Jul 31, 2020

6 tasks

📌 Commit 1b90e78393866dbedc859bd09a353ce83112e475 has been approved by withoutboats

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Aug 1, 2020

Is this intentional?

That submodule was noticed before, then came back with the tracking issues...

@bors r-

bors added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Aug 1, 2020

................what? sry

fixed it @bors r=withoutboats

📌 Commit d51b71a has been approved by withoutboats

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Aug 1, 2020

lcnr deleted the array_chunks branch

August 1, 2020 11:13

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 12, 2020

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Oct 22, 2021

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Oct 22, 2021

Implement split_array and split_array_mut

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

add slice::array_chunks to std by lcnr · Pull Request #74373 · rust-lang/rust (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

add `slice::array_chunks` to std by lcnr · Pull Request #74373 · rust-lang/rust (original) (raw)