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.

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

Stabilize Range[Inclusive]::is_empty #75132

Merged
merged 2 commits into from
Aug 25, 2020

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Aug 4, 2020

I would like to propose these two simple methods for stabilization:

  • Knowing that a range is exhausted isn't otherwise trivial
  • Clippy would like to suggest them, but had to do extra work to disable that path len_zero with Range suggests code cannot compile rust-clippy#3807 because they're unstable
  • These work on PartialOrd, consistently with the stable contains method, and are thus more general than iterator-based approaches that need Step
  • They've been unchanged for some time, and have picked up uses in the compiler
  • Stabilizing them doesn't block any future iterator-based is_empty plans, as these inherent ones are preferred in name resolution

https://doc.rust-lang.org/nightly/std/ops/struct.Range.html#method.is_empty
https://doc.rust-lang.org/nightly/std/ops/struct.RangeInclusive.html#method.is_empty

Closes #48111

@scottmcm scottmcm added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Aug 4, 2020
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2020
@scottmcm
Copy link
Member Author

scottmcm commented Aug 4, 2020

Since this needs an FCP, picking someone from libs who recently re-tagged the tracking issue:

r? @KodrAus

@scottmcm scottmcm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 4, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Aug 4, 2020

@rfcbot fcp merge

This proposes stabilizing the following API:

impl<Idx: PartialOrd<Idx>> Range<Idx> {
    pub fn is_empty(&self) -> bool;
}

impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
    pub fn is_empty(&self) -> bool;
}

@rfcbot
Copy link

rfcbot commented Aug 4, 2020

Team member @KodrAus has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 4, 2020
@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 4, 2020
@jonas-schievink jonas-schievink added this to the 1.47 milestone Aug 4, 2020
@scottmcm
Copy link
Member Author

@SimonSapin @sfackler @withoutboats Friendly ping for this pFCP.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Aug 13, 2020
@rfcbot
Copy link

rfcbot commented Aug 13, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 13, 2020
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 23, 2020
@rfcbot
Copy link

rfcbot commented Aug 23, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@Dylan-DPC-zz Dylan-DPC-zz modified the milestones: 1.47, 1.48 Aug 24, 2020
@scottmcm
Copy link
Member Author

Darn, missed 1.47 by a day, I guess. I've changed the stability attributes to 1.48.

@Mark-Simulacrum
Copy link
Member

Heh well we have not yet actually branched beta, that usually happens Tuesdays :)

But likely this wouldn't have landed in time anyway...

@dtolnay
Copy link
Member

dtolnay commented Aug 24, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 24, 2020

📌 Commit 78855479830ada08b210a483fdb34d07063d6192 has been approved by dtolnay

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2020
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2020
@scottmcm
Copy link
Member Author

scottmcm commented Aug 24, 2020

Doh, I mixed up - and _. That'll teach me to submit and trust the PR build instead of testing locally 😬

@scottmcm
Copy link
Member Author

@bors r=dtolnay

Clippy is passing locally, so 🤞
image

@bors
Copy link
Contributor

bors commented Aug 25, 2020

📌 Commit d6185f9 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 Aug 25, 2020
@bors
Copy link
Contributor

bors commented Aug 25, 2020

⌛ Testing commit d6185f9 with merge 7a279b300e1be073511080e53bfc0249716138dc...

@bors
Copy link
Contributor

bors commented Aug 25, 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 Aug 25, 2020
@scottmcm
Copy link
Member Author

Wow am I having good luck today:
image

@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 Aug 25, 2020
@bors
Copy link
Contributor

bors commented Aug 25, 2020

⌛ Testing commit d6185f9 with merge c30341d...

@bors
Copy link
Contributor

bors commented Aug 25, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: dtolnay
Pushing c30341d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 25, 2020
@bors bors merged commit c30341d into rust-lang:master Aug 25, 2020
@scottmcm scottmcm modified the milestones: 1.48, 1.47 Aug 25, 2020
@scottmcm scottmcm deleted the stabilize-range-is-empty branch August 25, 2020 08:55
bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 25, 2020
Re-enable len_zero for ranges now that `is_empty` is stable on them

Fixes #5956

Completed stabilization PR: rust-lang/rust#75132

changelog: len_zero: re-enable linting ranges now that range_is_empty is stable
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Aug 26, 2020
@dtolnay dtolnay self-assigned this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. 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
Development

Successfully merging this pull request may close these issues.

Tracking issue for Range[Inclusive]::is_empty (feature range_is_empty)