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 assoc_int_consts associated int/float constants #68952

Open
wants to merge 11 commits into
base: master
from

Conversation

@faern
Copy link
Contributor

faern commented Feb 8, 2020

The next step in RFC rust-lang/rfcs#2700 (tracking issue #68490). Stabilizing the associated constants that were added in #68325.

  • Stabilize all constants under the assoc_int_consts feature flag.
  • Update documentation on old constants to say they are soft-deprecated and the new ones should be preferred.
  • Update documentation examples to use new constants.
  • Remove uint_macro and use int_macro for all integer types since the macros were identical anyway.

r? @LukasKalbertodt

Copy link
Member

LukasKalbertodt left a comment

Great work, thanks!

(If you are comfortable with git rebase/squash/fixup you could improve the git history a bit by squashing your three "Fix ..." commits with the respective commits that is being fixed. That makes it a bit easier for commit-by-commit reviewing. However, that's really not important.)

@LukasKalbertodt

This comment has been minimized.

Copy link
Member

LukasKalbertodt commented Feb 9, 2020

The code changes are good. However, as I mentioned in chat, I am not really a fan of stabilizing these constants only a few weeks after they have been added. @Mark-Simulacrum disagreed. To get another opinion and since I can't start FCP merges anyway, reassigning:

r? @dtolnay

@faern faern force-pushed the faern:stabilize-assoc-int-consts branch from bdbbd71 to d06ed7e Feb 9, 2020
@faern

This comment has been minimized.

Copy link
Contributor Author

faern commented Feb 9, 2020

Git history fixed :) I usually do that, but must have been lazy this time. Thanks.

With regards to when to stabilize. My two cents is that things sit around as unstable because we are evaluating if the naming is good, the API has a usable/ergonomic design, it's properly implemented, people actually seem to use/want this etc. But we basically already know all those things for this feature. It just moves an existing feature.

For reference, moving the std::time::UNIX_EPOCH to SystemTime::UNIX_EPOCH assoc const (#49502) took two months from unstable to stable. But that included the RFC process, since it was added as unstable before any RFC was even posted. So not a fair comparison. These constants here have already been debated for nine months already :)

Copy link
Member

dtolnay left a comment

Thanks, I think it's pretty clear that we want these so I am on board with moving forward.

I think the deprecation notice on the integer modules is too aggressive for now.

Could you remove the bold second sentences? I think this is not among the most important information to feature in the module index, especially not with emphasis. I think the links inside the module showing what to use instead are sufficient.

Could you also restore the "See also the i16 primitive type." links exactly how it existed before? Anyone who lands on the module's documentation is much more likely looking for the type and its methods, not the associated constants. So the way you've put the type link at the end of a paragraph about deprecation warnings and associated constants is not as good.

@faern

This comment has been minimized.

Copy link
Contributor Author

faern commented Feb 10, 2020

Could you remove the bold second sentences?

Sure. Add it to a new line so it ends up in the module docs, but not in the first-line summary like now? Or remove This module is soft-deprecated completely and just keep the paragraph on how using these constants won't cause compilation warnings (for now) but new users should prefer the associated constants?

Could you also restore the "See also the i16 primitive type."

Will do.

@faern faern force-pushed the faern:stabilize-assoc-int-consts branch from d06ed7e to f086fb4 Feb 10, 2020
@faern

This comment has been minimized.

Copy link
Contributor Author

faern commented Feb 10, 2020

Done.

@dtolnay dtolnay added the needs-fcp label Feb 10, 2020
@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 10, 2020

@rfcbot fcp merge

@rust-lang/libs: This stabilizes associated constants on the primitive integer and float types to make e.g. i64::MAX work the way we've always liked for them to work, without importing std::i64.

The constants involved are:

  • On all the integers: MIN and MAX
  • On the floats: RADIX, MANTISSA_DIGITS, DIGITS, EPSILON, MIN, MIN_POSITIVE, MAX, MIN_EXP, MAX_EXP, MIN_10_EXP, MAX_10_EXP, NAN, INFINITY, NEG_INFINITY

The existing module-level constants' documentation will now link to the associated constants, but we don't trigger any new deprecation warnings. We can navigate deprecation at some (far) later time.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 10, 2020

Team member @dtolnay 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.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 11, 2020

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

@faern faern force-pushed the faern:stabilize-assoc-int-consts branch from f086fb4 to b2dc618 Feb 12, 2020
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 15, 2020

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

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 25, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.