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 slice take methods #88502

Merged
merged 1 commit into from
Dec 1, 2021
Merged

Add slice take methods #88502

merged 1 commit into from
Dec 1, 2021

Conversation

ibraheemdev
Copy link
Member

Revival of #62282

This PR adds the following slice methods:

  • take
  • take_mut
  • take_first
  • take_first_mut
  • take_last
  • take_last_mut

r? @LukasKalbertodt

@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented Aug 31, 2021 •
edited
Loading

As far as I can tell, LukasKalbertodt is no longer a member of the rust team, and that confused the automation. Assigning to a random libs-api member.

r? @dtolnay

@inquisitivecrystal inquisitivecrystal added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-slice Area: `[T]` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 31, 2021
@bors
Copy link
Contributor

bors commented Sep 3, 2021

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

@JohnCSimon
Copy link
Member

ping from triage:
@ibraheemdev
Returning to you to address merge conflicts and after that, please switch back to S-waiting-on-review.
thanks.

@rustbot label: +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2021
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 20, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 22, 2021

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

@crlf0710 crlf0710 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2021
@crlf0710
Copy link
Member

crlf0710 commented Oct 9, 2021

@ibraheemdev Ping from triage, CI is still red here.

@ibraheemdev ibraheemdev force-pushed the slice-take branch 3 times, most recently from 0c68175 to 3d54e0f Compare October 9, 2021 15:26
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 9, 2021
@WaffleLapkin
Copy link
Member

I want to mention that it's actually a fourth take on this feature. Previous attempts (chronological order, first to last):

  1. Introduce <&[_]>::split_off and <&str>::split_off #49173
  2. Add take_... functions to slices #62282
  3. Take 2: Add take_... functions to slices #77065

@bors
Copy link
Contributor

bors commented Oct 15, 2021

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

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2021
dtolnay
dtolnay approved these changes Dec 1, 2021
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good. It's hard for me to judge how well these &mut &'a Self signatures are going to work in practice, but I am excited for people to try it.

@dtolnay
Copy link
Member

dtolnay commented Dec 1, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 1, 2021

📌 Commit 8db85a3 has been approved by dtolnay

@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 Dec 1, 2021
@dtolnay dtolnay mentioned this pull request Dec 1, 2021
Open
6 tasks
AnthonyMikh
AnthonyMikh reviewed Dec 1, 2021
View reviewed changes
library/core/src/slice/mod.rs
(Unbounded, Included(i)) => (Direction::Front, i.checked_add(1)?),
(Excluded(i), Unbounded) => (Direction::Back, i.checked_add(1)?),
(Included(i), Unbounded) => (Direction::Back, *i),
_ => unreachable!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the presence of panic call here which optimiser will have to remove. Would it make more sense to make this a method of OneSidedRange in order to statically eliminate impossible cases?

@matthiaskrgr matthiaskrgr mentioned this pull request Dec 1, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 1, 2021
@bors
Auto merge of rust-lang#91433 - matthiaskrgr:rollup-118ql06, r=matthi…
48a5999 
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#88502 (Add slice take methods)
 - rust-lang#91313 (expand: Turn `ast::Crate` into a first class expansion target)
 - rust-lang#91424 (Update LLVM with patches for better llvm-cov diagnostics)
 - rust-lang#91425 (Include lint errors in error count for `-Ztreat-err-as-bug`)
 - rust-lang#91430 (Add tests for `normalize-docs` overflow errors)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9f1f428 into rust-lang:master Dec 1, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 1, 2021
RalfJung
RalfJung reviewed Dec 2, 2021
View reviewed changes
library/core/tests/slice.rs
slice: &[(); usize::MAX], method: take,
(take_in_bounds_max_range_to, (..usize::MAX), Some(EMPTY_MAX), &[(); 0]),
(take_oob_max_range_to_inclusive, (..=usize::MAX), None, EMPTY_MAX),
(take_in_bounds_max_range_from, (usize::MAX..), Some(&[] as _), EMPTY_MAX),
Copy link
Member

Choose a reason for hiding this comment

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

This test fails in Miri, and I am trying to find out why... help would be appreciated. Here's the Zulip thread.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, turns out that even with rustc this test diverges in debug mode, since it starts a loop of size usize::MAX. Miri doesn't optimize so it has no way to run this test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnthonyMikh AnthonyMikh AnthonyMikh left review comments

@RalfJung RalfJung RalfJung left review comments

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
A-slice Area: `[T]` 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.59.0
Development

Successfully merging this pull request may close these issues.