Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add array_windows fn #75026

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Add array_windows fn #75026

merged 1 commit into from
Sep 16, 2020
+196 −6

Conversation

JulianKnodt
Copy link
Contributor

@JulianKnodt JulianKnodt commented Aug 1, 2020 •
edited
Loading

This mimicks the functionality added by array_chunks, and implements a const-generic form of
windows. It makes egregious use of unsafe, but by necessity because the array must be
re-interpreted as a slice of arrays, and unlike array_chunks this cannot be done by casting the
original array once, since each time the index is advanced it needs to move one element, not
N.

I'm planning on adding more tests, but this should be good enough as a premise for the functionality.
Notably: should there be more functions overwritten for the iterator implementation/in general?

I've marked the issue as #74985 as there is no corresponding exact issue for array_windows, but it's based of off array_chunks.

Edit: See Issue #75027 created by @lcnr for tracking issue

Do not merge until I add more tests, please.

r? @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2020
@lcnr lcnr mentioned this pull request Aug 1, 2020
Open
3 tasks
@lcnr
Copy link
Contributor

lcnr commented Aug 1, 2020

Added a separate tracking issue for array_windows: #75027

Please update the the links for array_windows here

lcnr
lcnr reviewed Aug 1, 2020
View reviewed changes
library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
lcnr
lcnr reviewed Aug 1, 2020
View reviewed changes
library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
@JulianKnodt
Copy link
Contributor Author

JulianKnodt commented Aug 2, 2020 •
edited
Loading

Would it make sense to define next and next_back based on nth and nth_back? They're both just nth(0) and nth_back(0) respectively

Edit: decided to go with defining in terms of this, just reduces the code size and centralizes the logic.

@lcnr
Copy link
Contributor

lcnr commented Aug 2, 2020

Looks fairly good to me.

It might be more intuitive to only define next and next_back instead and implement nth by self.num.saturating_sub(n); self.next().

I personally don't care either way here.

Not part of T-libs though, so a as final review: @bors r? @withoutboats

@rust-highfive rust-highfive assigned withoutboats and unassigned lcnr Aug 2, 2020
lcnr
lcnr reviewed Aug 2, 2020
View reviewed changes
library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

Sign in to view
@LukasKalbertodt LukasKalbertodt mentioned this pull request Aug 13, 2020
Merged
@JulianKnodt JulianKnodt force-pushed the array_windows branch 5 times, most recently from eec9e0c to 858e817 Compare August 13, 2020 19:52
@cuviper cuviper mentioned this pull request Sep 5, 2020
Open
@bors

This comment has been minimized.

Sign in to view
@JulianKnodt
Copy link
Contributor Author

This seems to be a spurious error, is it possible for tests to be rerun?

@Dylan-DPC-zz
Copy link

r? @Amanieu

@jyn514 jyn514 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-const-generics Area: const generics (parameters and arguments) labels Sep 15, 2020
@bors
Copy link
Contributor

bors commented Sep 15, 2020

☔ The latest upstream changes (presumably #76311) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@JulianKnodt JulianKnodt force-pushed the array_windows branch 12 times, most recently from a5fc8d3 to 8af9677 Compare September 16, 2020 06:38
@JulianKnodt
Add array window fn
f240abc 
Updated issue to rust-lang#75027

Update to rm oob access

And hopefully fix docs as well

Fixed naming conflict in test

Fix test which used 1-indexing

Nth starts from 0, woops

Fix a bunch of off by 1 errors

See https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=757b311987e3fae1ca47122969acda5a

Add even more off by 1 errors

And also write `next` and `next_back` in terms of `nth` and `nth_back`.

Run fmt

Fix forgetting to change fn name in test

add nth_back test & document unsafe

Remove as_ref().unwrap()
Documented occurrences of unsafe, noting what invariants are maintained
@Amanieu
Copy link
Member

Amanieu commented Sep 16, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 16, 2020

📌 Commit f240abc has been approved by Amanieu

@bors 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 Sep 16, 2020
@tmandry tmandry mentioned this pull request Sep 16, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 16, 2020
@bors
Auto merge of rust-lang#76804 - tmandry:rollup-nwntt3q, r=tmandry
285fc7d 
Rollup of 16 pull requests

Successful merges:

 - rust-lang#75026 (Add array_windows fn)
 - rust-lang#76642 (Do not lint ignored private doc tests)
 - rust-lang#76719 (Change error message for ty param in const)
 - rust-lang#76721 (Use intra-doc links in `core::mem`)
 - rust-lang#76728 (Add a comment why `extern crate` is necessary for rustdoc)
 - rust-lang#76735 (Remove unnecessary `clone()`s in bootstrap)
 - rust-lang#76741 (Avoid printing dry run timings)
 - rust-lang#76747 (Add missing code examples in libcore)
 - rust-lang#76756 (fix a couple of stylistic clippy warnings)
 - rust-lang#76758 ([fuchsia] Propagate the userspace UTC clock)
 - rust-lang#76759 (Fix stabilization marker for future_readiness_fns)
 - rust-lang#76760 (don't lazily evaluate some trivial values for Option::None replacements (clippy::unnecessary_lazy_evaluations))
 - rust-lang#76764 (Update books)
 - rust-lang#76775 (Strip a single leading tab when rendering dataflow diffs)
 - rust-lang#76778 (Simplify iter fuse struct doc)
 - rust-lang#76794 (Make graphviz font configurable)

Failed merges:

r? `@ghost`
@bors bors merged commit 23a6777 into rust-lang:master Sep 16, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 16, 2020
@lcnr lcnr mentioned this pull request Nov 17, 2020
Closed
16 tasks
@jethrogb jethrogb mentioned this pull request Mar 17, 2021
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 22, 2021
@matthiaskrgr
Rollup merge of rust-lang#83233 - jethrogb:split_array, r=yaahc
301a4f2 
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
@matthiaskrgr
Rollup merge of rust-lang#83233 - jethrogb:split_array, r=yaahc
aa9a445 
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 23, 2021
@matthiaskrgr
Rollup merge of rust-lang#83233 - jethrogb:split_array, r=yaahc
5ea0274 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Amanieu Amanieu Amanieu left review comments

@lcnr lcnr lcnr left review comments

Assignees

@Amanieu Amanieu

Labels
A-const-generics Area: const generics (parameters and arguments) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@JulianKnodt @lcnr @bors @Dylan-DPC-zz @Amanieu @rust-highfive @withoutboats @jyn514 @rustbot