Skip to content

Move slice::check_range to RangeBounds#76885

Merged
bors merged 4 commits intorust-lang:masterfrom
dylni:move-slice-check-range-to-range-bounds
Oct 18, 2020
Merged

Move slice::check_range to RangeBounds#76885
bors merged 4 commits intorust-lang:masterfrom
dylni:move-slice-check-range-to-range-bounds

Conversation

@dylni
Copy link
Contributor

@dylni dylni commented Sep 18, 2020 •
edited
Loading

Since this method doesn't take a slice anymore (#76662), it makes more sense to define it on RangeBounds.

Questions:

  • Should the new method be assert_len or assert_length?

pickfire
pickfire reviewed Sep 18, 2020
View reviewed changes
Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

Naming for_length looks non-intuitive but the movement looks good.

@dylni
Copy link
Contributor Author

dylni commented Sep 18, 2020

I don't like the name either. What do you think of check_for_length?

@pickfire
Copy link
Contributor

pickfire commented Sep 18, 2020 •
edited
Loading

I don't like the name either. What do you think of check_for_length?

Since this is doing bound checking, how about naming it check_bounds or check_len or assert_len (this may be weirder) which is exactly what is does? I wonder if there are other better names.

@dylni
Copy link
Contributor Author

dylni commented Sep 18, 2020

Since this is doing bound checking, how about naming it check_bounds or check_len or assert_len (this may be weirder) which is exactly what is does? I wonder if there are other better names.

assert_len is a really great suggestion. I'll use that name.

@dylni
Copy link
Contributor Author

dylni commented Sep 18, 2020

@rust-highfive missed this PR, so randomly assigning to a libs member.

r? @joshtriplett

@joshtriplett
Copy link
Member

joshtriplett commented Sep 21, 2020

@dylni I'm not a libs member.

@dylni
Copy link
Contributor Author

dylni commented Sep 21, 2020

Sorry @joshtriplett. I was looking at this file.

r? @KodrAus who reviewed the initial PR.

@joshtriplett
Copy link
Member

joshtriplett commented Sep 24, 2020

@dylni Interesting, thanks for pointing me at that. :)

@dylni
Copy link
Contributor Author

dylni commented Sep 25, 2020

Not a problem. :) It's good to know I'm looking in the right place

@LeSeulArtichaut LeSeulArtichaut added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2020
This was referenced Oct 3, 2020
Closed
Open
@KodrAus
Copy link
Contributor

KodrAus commented Oct 5, 2020

Since this method doesn't take a slice anymore (#76662)

Ah yikes! I'm going to have to spend some time digging into that regression properly.

This extra refactoring looks good to me though.

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 5, 2020

📌 Commit f055b0b has been approved by KodrAus

@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 Oct 5, 2020
@dylni
Copy link
Contributor Author

dylni commented Oct 5, 2020

@KodrAus Thanks!

Ah yikes! I'm going to have to spend some time digging into that regression properly.

I was surprised too. Ralf explains it well here.

@bors
Copy link
Collaborator

bors commented Oct 5, 2020

⌛ Testing commit f055b0b with merge a19dde735bd9a7faeb838e2e786d8b5e884ddff1...

@bors
Copy link
Collaborator

bors commented Oct 5, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 5, 2020
@dylni
Copy link
Contributor Author

dylni commented Oct 5, 2020

@bors retry

@bors
Copy link
Collaborator

bors commented Oct 5, 2020

@dylni: 🔑 Insufficient privileges: not in try users

@joshtriplett
Copy link
Member

joshtriplett commented Oct 5, 2020

@bors retry

@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 Oct 5, 2020
@bors
Copy link
Collaborator

bors commented Oct 6, 2020

⌛ Testing commit f055b0b with merge d04e136f6e752db206b94dfe266a9a5eeeb42424...

@bors
Copy link
Collaborator

bors commented Oct 6, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 6, 2020
@dylni
Copy link
Contributor Author

dylni commented Oct 6, 2020

The failure still doesn't look related to this PR.

@dylni
Copy link
Contributor Author

dylni commented Oct 8, 2020

@joshtriplett Can you retry bors again?

@jyn514
Copy link
Member

jyn514 commented Oct 18, 2020

GitHub Actions has encountered an internal error when running your job.

@bors retry

@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 Oct 18, 2020
@bors
Copy link
Collaborator

bors commented Oct 18, 2020

⌛ Testing commit f055b0b with merge 187b877...

@bors
Copy link
Collaborator

bors commented Oct 18, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: KodrAus
Pushing 187b877 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 18, 2020
@bors bors merged commit 187b877 into rust-lang:master Oct 18, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 18, 2020
This was referenced Oct 18, 2020
Closed
Closed
Closed
@dylni dylni deleted the move-slice-check-range-to-range-bounds branch October 19, 2020 01:35
vetio pushed a commit to vetio/volatile that referenced this pull request Oct 31, 2020
Changed slice::check_range to RangeBounds::assert_len.
d110f5d 
In 1.49 the slice::check_range unstable funciton got moved to
RangeBounds::assert_len.
rust-lang/rust#76885
@vetio vetio mentioned this pull request Oct 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@pickfire pickfire pickfire left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@KodrAus KodrAus

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.49.0

Development

Successfully merging this pull request may close these issues.

8 participants

@dylni @pickfire @joshtriplett @KodrAus @bors @jyn514 @LeSeulArtichaut @rustbot