Skip to content

Add slice take methods#88502

Merged
bors merged 1 commit into
rust-lang:masterfrom
ibraheemdev:slice-take
Dec 1, 2021
Merged

Add slice take methods#88502
bors merged 1 commit into
rust-lang:masterfrom
ibraheemdev:slice-take

Conversation

@ibraheemdev

@ibraheemdev ibraheemdev commented Aug 30, 2021

Copy link
Copy Markdown
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

inquisitivecrystal commented Aug 31, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

bors commented Sep 3, 2021

Copy link
Copy Markdown
Collaborator

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

@JohnCSimon

JohnCSimon commented Sep 20, 2021

Copy link
Copy Markdown
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

bors commented Sep 22, 2021

Copy link
Copy Markdown
Collaborator

☔ 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

crlf0710 commented Oct 9, 2021

Copy link
Copy Markdown
Member

@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

WaffleLapkin commented Oct 12, 2021

Copy link
Copy Markdown
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

bors commented Oct 15, 2021

Copy link
Copy Markdown
Collaborator

☔ 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

@dtolnay dtolnay left a comment

Copy link
Copy Markdown
Member

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

dtolnay commented Dec 1, 2021

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Dec 1, 2021

Copy link
Copy Markdown
Collaborator

📌 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
Closed
7 tasks
AnthonyMikh
AnthonyMikh reviewed Dec 1, 2021
View reviewed changes
Comment thread 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!(),

@AnthonyMikh AnthonyMikh Dec 1, 2021

Copy link
Copy Markdown
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
Comment thread 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),

@RalfJung RalfJung Dec 2, 2021

Copy link
Copy Markdown
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.

@RalfJung RalfJung Dec 2, 2021

Copy link
Copy Markdown
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.

@tgross35 tgross35 mentioned this pull request Feb 9, 2026
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments

@dtolnay dtolnay dtolnay approved these changes

+1 more reviewer

@AnthonyMikh AnthonyMikh AnthonyMikh left review comments

Reviewers whose approvals may not affect merge requirements

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.