Make the getter for NonZero types into a const fn #56739

Lokathor · 2018-12-12T06:25:57Z

I opened an issue for this here earlier today and no one immediately called it foolish, so here's the PR for it.

rust-highfive · 2018-12-12T06:26:00Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

oli-obk · 2018-12-12T09:23:42Z

Previous attempt: #54799

The other PR also has a bunch of tests and related changes. If you have some spare time, it would be great if you could just rebase that PR and carry it over the finish line.

Lokathor · 2018-12-12T09:25:29Z

Wasn't aware of the other PR. I'll attempt that as soon as I can find the time.

kennytm · 2018-12-12T14:13:36Z

r? @oli-obk

Lokathor · 2018-12-12T15:46:41Z

Near as I can tell, several of the changes involved adding a special feature

#![feature(const_nonzero_methods)]

which we don't need to bother with now that the minimal const fn is stable for everybody. Does that sound right @oli-obk ? I can get to doing this later today after work.

oli-obk · 2018-12-12T15:59:37Z

which we don't need to bother with now that the minimal const fn is stable for everybody. Does that sound right @oli-obk ?

We still need to do that. Just like any other feature, we first implement it behind a feature gate, and then, once it has been tested on nightly, we stabilize the feature.

Lokathor · 2018-12-12T22:30:58Z

I added the test and gate and such for NonZero types, but I didn't touch DebruijnIndex or any other thing at all. I don't have any understanding of those other parts and I don't want to get lots in the weeds. I'm focusing on a small PR that's a clear improvement, and other things can come later.

Centril · 2018-12-13T00:30:12Z

which we don't need to bother with now that the minimal const fn is stable for everybody. Does that sound right @oli-obk ?

We still need to do that. Just like any other feature, we first implement it behind a feature gate, and then, once it has been tested on nightly, we stabilize the feature.

I think we can dispense with that since we have min_const_fn on stable now; imo someone from T-libs can just FCP instead.

cc @SimonSapin

SimonSapin · 2018-12-13T06:46:03Z

I’m in favor of making this constness stable immediately.

@rfcbot fcp merge

Lokathor · 2018-12-13T06:47:27Z

Should I do one more commit to remove the #[rustc_const_unstable(feature = "const_nonzero_methods")] then?

SimonSapin · 2018-12-13T06:53:16Z

@Lokathor Either that, or force-push your branch to remove the commit that adds it. But maybe only after we get team consensus, in case someone thinks differently.

With the team label:

@rfcbot fcp merge

rfcbot · 2018-12-13T06:53:16Z

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

alexcrichton · 2018-12-13T16:27:23Z

It's not 100% clear to me if checking a box to stabilize this as const or just the idea that it may one day be stable with const, but I'm totally fine merging this as stable const with team consensus

dtolnay · 2018-12-13T16:51:20Z

As implemented in the PR, if the constness is unstable why did feature(const_nonzero_methods) need to get added in librustc / librustc_data_structures / librustc_mir? Would those crates not have compiled without feature(const_nonzero_methods) i.e. is this a breaking change? Those crates can't already have been using NonZeroT::get in const context since it wasn't const fn. If they were using NonZeroT::get but not in const context then why does that require enabling feature(const_nonzero_methods)?

oli-obk · 2018-12-13T16:56:22Z

Would those crates not have compiled without feature(const_nonzero_methods) i.e. is this a breaking change?

This is just a leftover from minimizing #54799, which actually used the function in a const way. I did not comment on that, because it was going to go away anyway if we choose to directly stabilize.

SimonSapin · 2018-12-13T16:58:41Z

In #56739 (comment) I proposed FCP to stabilize as const, without a feature gate. If we have team consensus, the PR would need to change before merging.

bors · 2018-12-18T00:49:20Z

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

Lokathor · 2018-12-18T01:47:36Z

The merge conflict is simple to fix but we're waiting on 1 more vote before deciding on instant stable or nightly with feature. I don't want to clutter up the commit list, so I'll wait for that vote and decision.

But thanks bors-bot.

Ravenslofty · 2018-12-22T23:58:53Z

Ping from not-triage: can @Kimundi, @sfackler or @withoutboats review this? We just need one more tick for a majority.

rfcbot · 2018-12-23T01:44:51Z

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

Lokathor · 2018-12-23T03:41:05Z

@SimonSapin so it seems like we're going for the instant-stable option, correct?

oli-obk · 2018-12-23T12:30:25Z

yes, feel free to proactively update the PR if you want

Lokathor · 2018-12-23T18:36:30Z

@oli-obk all set.

bors · 2018-12-27T14:40:06Z

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

Lokathor · 2018-12-27T21:27:14Z

@SimonSapin why would you do this to me on the day that my ISP stuffers a national outage! :P

I will rebase when I can.

Centril · 2018-12-28T06:29:38Z

@Lokathor did the rebase go wrong?

Lokathor · 2018-12-28T06:32:39Z

It said there was nothing to rebase? Maybe I typed in the git command wrong. The "resolve conflicts" tool seems to have sorted it out though.

Centril · 2018-12-28T06:34:55Z

@Lokathor You have a merge-commit tho and we prefer not to have those... ;)

SimonSapin · 2018-12-28T12:14:57Z

Ehh sorry? I can take care of rebasing for you if you’d like. Let me know, and make sure you check the checkbox https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/#enabling-repository-maintainer-permissions-on-existing-pull-requests

Dylan-DPC-zz · 2018-12-28T12:17:16Z

Reworked this in #57167

@Lokathor

Make the getter for NonZero types into a const fn Closes #53331 Rework of #56739 cc @Lokathor r? @oli-obk

Make the getter for NonZero types into a const fn

d314dd3

rust-highfive assigned kennytm Dec 12, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 12, 2018

oli-obk assigned oli-obk and unassigned kennytm Dec 12, 2018

Lokathor added 2 commits December 12, 2018 15:18

Add the approrpiate test

f2f32fe

Added the appropriate feature gate and feature use

6a02305

This comment has been minimized.

Sign in to view

added the trailing newline as demanded by tidy

55593eb

SimonSapin added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 13, 2018

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 Dec 13, 2018

prep-commit for insta-stable

f341d71

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 23, 2018

no feature gate required

566cf71

oli-obk approved these changes Dec 23, 2018

View reviewed changes

Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 25, 2018

Dylan-DPC-zz mentioned this pull request Dec 28, 2018

Make the getter for NonZero types into a const fn #57167

Merged

Dylan-DPC-zz closed this Dec 28, 2018

Dylan-DPC-zz removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2018

bors added a commit that referenced this pull request Dec 28, 2018

Auto merge of #57167 - Dylan-DPC:feature/non-zero-getters, r=SimonSapin

3cda631

Make the getter for NonZero types into a const fn Closes #53331 Rework of #56739 cc @Lokathor r? @oli-obk

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 2, 2019

kennytm mentioned this pull request Feb 8, 2019

Make overflowing and wrapping negation const #58044

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the getter for NonZero types into a const fn #56739

Make the getter for NonZero types into a const fn #56739

Lokathor commented Dec 12, 2018

rust-highfive commented Dec 12, 2018

oli-obk commented Dec 12, 2018

Lokathor commented Dec 12, 2018

kennytm commented Dec 12, 2018

Lokathor commented Dec 12, 2018

oli-obk commented Dec 12, 2018

Lokathor commented Dec 12, 2018

This comment has been minimized.

Centril commented Dec 13, 2018

SimonSapin commented Dec 13, 2018

Lokathor commented Dec 13, 2018

SimonSapin commented Dec 13, 2018

rfcbot commented Dec 13, 2018 •

edited by sfackler

Loading

alexcrichton commented Dec 13, 2018

dtolnay commented Dec 13, 2018

oli-obk commented Dec 13, 2018

SimonSapin commented Dec 13, 2018

bors commented Dec 18, 2018

Lokathor commented Dec 18, 2018

Ravenslofty commented Dec 22, 2018

rfcbot commented Dec 23, 2018

Lokathor commented Dec 23, 2018 •

edited

Loading

oli-obk commented Dec 23, 2018

Lokathor commented Dec 23, 2018

bors commented Dec 27, 2018

Lokathor commented Dec 27, 2018

Centril commented Dec 28, 2018

Lokathor commented Dec 28, 2018

Centril commented Dec 28, 2018

SimonSapin commented Dec 28, 2018

Dylan-DPC-zz commented Dec 28, 2018

Make the getter for NonZero types into a const fn #56739

Make the getter for NonZero types into a const fn #56739

Conversation

Lokathor commented Dec 12, 2018

rust-highfive commented Dec 12, 2018

oli-obk commented Dec 12, 2018

Lokathor commented Dec 12, 2018

kennytm commented Dec 12, 2018

Lokathor commented Dec 12, 2018

oli-obk commented Dec 12, 2018

Lokathor commented Dec 12, 2018

This comment has been minimized.

Centril commented Dec 13, 2018

SimonSapin commented Dec 13, 2018

Lokathor commented Dec 13, 2018

SimonSapin commented Dec 13, 2018

rfcbot commented Dec 13, 2018 • edited by sfackler Loading

alexcrichton commented Dec 13, 2018

dtolnay commented Dec 13, 2018

oli-obk commented Dec 13, 2018

SimonSapin commented Dec 13, 2018

bors commented Dec 18, 2018

Lokathor commented Dec 18, 2018

Ravenslofty commented Dec 22, 2018

rfcbot commented Dec 23, 2018

Lokathor commented Dec 23, 2018 • edited Loading

oli-obk commented Dec 23, 2018

Lokathor commented Dec 23, 2018

bors commented Dec 27, 2018

Lokathor commented Dec 27, 2018

Centril commented Dec 28, 2018

Lokathor commented Dec 28, 2018

Centril commented Dec 28, 2018

SimonSapin commented Dec 28, 2018

Dylan-DPC-zz commented Dec 28, 2018

rfcbot commented Dec 13, 2018 •

edited by sfackler

Loading

Lokathor commented Dec 23, 2018 •

edited

Loading