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

Make some Ordering methods const #75463

Merged
merged 4 commits into from
Aug 31, 2020
Merged

Make some Ordering methods const #75463

merged 4 commits into from
Aug 31, 2020

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Aug 12, 2020 •
edited
Loading

Constify the following methods of core::cmp::Ordering:

  • reverse
  • then

Possible because of #49146 (Allow if and match in constants).

Tracking issue: #76113

Make some Ordering methods const
6b0d44e 
Constify the following methods of `core::cmp::Ordering`:
 - `reverse`
 - `then`

Possible because of rust-lang#49146 (Allow `if` and `match` in constants).
@CDirkx
Copy link
Contributor Author

CDirkx commented Aug 12, 2020

What are the requirements for making functions const_stable or const_unstable?
Would this change require a const_ordering feature gate?

@crlf0710
Copy link
Member

@CDirkx Ping from triage, this needs to get the CI green before review. You'll need to use the #[rustc_const_unstable] attribute. Search within the code, and follow the existing examples will be enough.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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 Aug 28, 2020
@crlf0710 crlf0710 requested a review from sfackler August 28, 2020 17:55
@ecstatic-morse
Copy link
Contributor

There's no need for these to be unstable IMO now that if and match are stable in a const fn. It's not like these impls are going to change.

@CDirkx
Copy link
Contributor Author

CDirkx commented Aug 30, 2020

Yeah that is my question, is it enough that #49146 is stabilized to make these methods const_stable, since they are only simple matches? If so, that would also apply to a lot of the methods under const_option (#67441) and const_result ( #67520), as well as some integer methods I believe. I would be happy to make some stabilization PRs for those if that's the descision.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Aug 30, 2020 •
edited
Loading

As I understand it, you're correct and all those methods are eligible for stabilization. (cc @rust-lang/wg-const-eval in case there's an objection). I'll check in with t-libs as well.

In the meantime, I'll happily sign off on the insta-stable version. I guess you just keep the current feature name even though it never did anything?

r? @ecstatic-morse

@ecstatic-morse
Copy link
Contributor

@bors delegate+

r=me with correct release number (1.46 came out this week).

@bors
Copy link
Collaborator

bors commented Aug 30, 2020

✌️ @CDirkx can now approve this pull request

Update since to correct release
12f4624 
`const_ordering` will stabilize in version 1.48.0
@CDirkx
Copy link
Contributor Author

CDirkx commented Aug 30, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 30, 2020

📌 Commit 12f4624 has been approved by CDirkx

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2020
This was referenced Aug 31, 2020
Merged
Merged
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Aug 31, 2020
Closed
@bors
Copy link
Collaborator

bors commented Aug 31, 2020

⌛ Testing commit 12f4624 with merge 92290d1...

@bors
Copy link
Collaborator

bors commented Aug 31, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: CDirkx
Pushing 92290d1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 31, 2020
@bors bors merged commit 92290d1 into rust-lang:master Aug 31, 2020
@CDirkx CDirkx deleted the ordering-const branch August 31, 2020 03:37
@tesuji
Copy link
Contributor

tesuji commented Aug 31, 2020 •
edited
Loading

@rustbot modify labels: T-libs

@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2020

Error: Label relnotes can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 31, 2020
@RalfJung
Copy link
Member

RalfJung commented Aug 31, 2020 •
edited
Loading

Eh, shouldn't there be t-libs FCP on this? I don't think there is precedent for insta-stabilizing new const, but the methods seem simple enough. Still, this needs team approval.

Cc @rust-lang/libs

Also @CDirkx, please don't r+ unless you are the reviewer. When you approve in the name of someone else, please do r=<name of reviewer>. (We use r=me as a short-hand to say exactly that, but it is entire incomprehensible unless you already know what it means.^^)

@RalfJung RalfJung added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 31, 2020
@CDirkx
Copy link
Contributor Author

CDirkx commented Aug 31, 2020

@RalfJung Ah, yeah I was unsure if that was the intention. Thanks!

@Dylan-DPC-zz Dylan-DPC-zz added this to the 1.48 milestone Aug 31, 2020
@tmandry tmandry mentioned this pull request Sep 1, 2020
Merged
@CDirkx CDirkx restored the ordering-const branch September 1, 2020 02:22
@CDirkx CDirkx deleted the ordering-const branch September 1, 2020 02:24
@RalfJung RalfJung removed the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 1, 2020
@RalfJung RalfJung removed this from the 1.48 milestone Sep 1, 2020
@CDirkx CDirkx mentioned this pull request Sep 1, 2020
Merged
@pnkfelix pnkfelix mentioned this pull request Oct 14, 2022
Closed
@cuviper cuviper added this to the 1.48.0 milestone May 2, 2024
@RalfJung RalfJung added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecstatic-morse ecstatic-morse

@sfackler sfackler

Assignees

@ecstatic-morse ecstatic-morse

Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@CDirkx @crlf0710 @ecstatic-morse @bors @tesuji @rustbot @RalfJung @cuviper @Dylan-DPC-zz @jyn514