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

Make most std::ops traits const on numeric types #89876

Merged
merged 2 commits into from
Oct 30, 2021

Conversation

AlexApps99
Copy link
Contributor

This PR makes existing implementations of std::ops traits (Add, Sub, etc) impl const where possible.
This affects:

  • All numeric primitives (u*, i*, f*)
  • NonZero*
  • Wrapping

This is under the rustc_const_unstable feature const_ops.
I will write tests once I know what can and can't be kept for the final version of this PR.

Since this is my first PR to rustc (and hopefully one of many), please give me feedback on how to better handle the PR process wherever possible. Thanks

Zulip discussion

@rust-highfive
Copy link
Collaborator

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 14, 2021
@oli-obk oli-obk added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 14, 2021
@bors
Copy link
Contributor

bors commented Oct 20, 2021

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

@AlexApps99
Copy link
Contributor Author

Looks like someone's PR beat me to it (#90009).
A bit disheartening, but only a small part of this PR.
Hopefully it can be reviewed soon.
In the mean time, I am writing a tracking issue.

@AlexApps99 AlexApps99 mentioned this pull request Oct 20, 2021
4 tasks
@AlexApps99 AlexApps99 marked this pull request as ready for review October 20, 2021 02:40
@oli-obk oli-obk added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Oct 20, 2021
@oli-obk oli-obk removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Oct 20, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2021

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned m-ou-se Oct 20, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2021

As discussed in the libs-api meeting today, @rust-lang/wg-const-eval may now approve unstably adding const to trait impls, including using ~const.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The impl looks good! We can go ahead with all of these. Just needs those tests you talked about.

@AlexApps99
Copy link
Contributor Author

Starting tests now, are the files library/core/tests/{nonzero,num/{int_macros,ops,uint_macros,wrapping}}.rs good places to start?

@fee1-dead
Copy link
Member

Starting tests now, are the files library/core/tests/{nonzero,num/{int_macros,ops,uint_macros,wrapping}}.rs good places to start?

Yes.

rust-lang/wg-const-eval may now approve unstably adding const to trait impls, including using ~const.

We should not approve ~const usages yet, since it kind of messes up the predicate evaluation cache. The ParamEnv fix should get landed first, and then we allow ~const.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 21, 2021

Ah yea, that is next on my list anyway, might have a PR up today

@bors
Copy link
Contributor

bors commented Oct 21, 2021

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

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 21, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 30, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 30, 2021

📌 Commit 361c978 has been approved by oli-obk

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 30, 2021
@AlexApps99
Copy link
Contributor Author

I haven't completed the tests yet (mostly for a lack of time), are you sure it's OK to be merged without them?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 30, 2021
Make most std::ops traits const on numeric types

This PR makes existing implementations of `std::ops` traits (`Add`, `Sub`, etc) [`impl const`](rust-lang#67792) where possible.
This affects:
- All numeric primitives (`u*`, `i*`, `f*`)
- `NonZero*`
- `Wrapping`

This is under the `rustc_const_unstable` feature `const_ops`.
I will write tests once I know what can and can't be kept for the final version of this PR.

Since this is my first PR to rustc (and hopefully one of many), please give me feedback on how to better handle the PR process wherever possible. Thanks

[Zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Const.20std.3A.3Aops.20traits.20PR)
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 30, 2021
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#89876 (Make most std::ops traits const on numeric types)
 - rust-lang#90371 (Fix incorrect doc link)
 - rust-lang#90374 (Unify titles in rustdoc book doc attributes chapter)
 - rust-lang#90377 (Make `core::slice::from_raw_parts[_mut]` const)
 - rust-lang#90395 (Restrict liveness of mutable borrow of inner infcx in ConstInferUnifier::consts)
 - rust-lang#90396 (Prevent type flags assertions being thrown in default_anon_const_substs if errors occurred)
 - rust-lang#90402 (Add a few query descriptions)
 - rust-lang#90412 (Remove unnecessary `macro_use`s in rustdoc)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 20bb932 into rust-lang:master Oct 30, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 30, 2021
@RalfJung
Copy link
Member

RalfJung commented Nov 3, 2021

I haven't completed the tests yet (mostly for a lack of time), are you sure it's OK to be merged without them?

Could you open an issue to track adding those tests, maybe leaving some notes for what kinds of tests you think would be good to have?

@AlexApps99
Copy link
Contributor Author

I'll put it on the tracking issue, I'm busy with exams right now but I should be able to set aside some time to implement the tests in a week or two.

@ChaiTRex
Copy link
Contributor

ChaiTRex commented Jul 24, 2022

This seems to add const to u8 >> i32, but not to &u8 >> i32, even though there's an implementation for both.

@fee1-dead
Copy link
Member

This seems to add const to u8 >> i32, but not to &u8 >> i32, even though there's an implementation for both.

Does this work?

@ChaiTRex
Copy link
Contributor

This seems to add const to u8 >> i32, but not to &u8 >> i32, even though there's an implementation for both.

Does this work?

Ahh, I see the problem. I was using #![feature(const_ops)] instead of #![feature(const_trait_impl)]. The latter works. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants