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

Move slice::check_range to RangeBounds #76885

Merged
merged 4 commits into from
Oct 18, 2020
Merged

Move slice::check_range to RangeBounds #76885

merged 4 commits into from
Oct 18, 2020

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

@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

@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
Contributor

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
Contributor

bors commented Oct 5, 2020

⌛ Testing commit f055b0b with merge a19dde735bd9a7faeb838e2e786d8b5e884ddff1...

@bors
Copy link
Contributor

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
Contributor

bors commented Oct 5, 2020

@dylni: 🔑 Insufficient privileges: not in try users

@joshtriplett
Copy link
Member

@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
Contributor

bors commented Oct 6, 2020

⌛ Testing commit f055b0b with merge d04e136f6e752db206b94dfe266a9a5eeeb42424...

@bors
Copy link
Contributor

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
Contributor

bors commented Oct 18, 2020

⌛ Testing commit f055b0b with merge 187b877...

@bors
Copy link
Contributor

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

@pickfire pickfire pickfire left review comments

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