-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Constify remaining traits/impls for const_ops
#143949
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
Conversation
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
97d85a1
to
d7a8770
Compare
This comment has been minimized.
This comment has been minimized.
d7a8770
to
ad7b7c4
Compare
ad7b7c4
to
071b5f0
Compare
Only organization members can add or resolve concerns. |
This comment has been minimized.
This comment has been minimized.
@rustbot concern May break Clippy (checking if concerns work as top-level comments only, real concern was written here, and discussion about the error message is on Zulip) |
@rustbot resolve May break Clippy We cleared up the misunderstanding of what the problem was. |
071b5f0
to
ed0722b
Compare
This comment has been minimized.
This comment has been minimized.
Yeah, I'll be honest, I have genuinely no idea what those tests are doing. I could just add the feature flag, but it wasn't necessary before, and I don't know why. |
Didn't you just forget to replace |
Last time I checked this file, it didn't have it, but now it does. 🤷🏻 Literally tried |
ed0722b
to
8e2f97b
Compare
TBH I think it'd be fine to have arith and bit ops in the same feature gate. We don't need a dozen of them, and it seems likely we'd stabilize these together. |
@bors r+ |
…nieu Constify remaining traits/impls for `const_ops` Tracking issue: rust-lang#143802 This is split into two commits for ease of reviewability: 1. Updates the `forward_ref_*` macros to accept multiple attributes (in anticipation of needing `rust_const_unstable` attributes) and also *require* attributes in these macros. Since the default attribute only helps for the initial implementations, it means it's easy to get wrong for future implementations, as shown for the saturating implementations which were incorrect before. 2. Actually constify the traits/impls. A few random other notes on the implementation specifically: * I unindented the attributes that were passed to the `forward_ref_*` macro calls because in some places rustfmt wanted them to be unindented, and in others it was allowed because they were themselves inside of macro bodies. I chose the consistent indenting even though I (personally) think it looks worse. ---- As far as the actual changes go, this constifies the following additional traits: * `Neg` * `Not` * `BitAnd` * `BitOr` * `BitXor` * `Shl` * `Shr` * `AddAssign` * `SubAssign` * `MulAssign` * `DivAssign` * `RemAssign` * `BitAndAssign` * `BitOrAssign` * `BitXorAssign` * `ShlAssign` * `ShrAssign` In terms of constified implementations of these traits, it adds the reference-forwarded versions of all the arithmetic operators, which are defined by the macros in `library/core/src/internal_macros.rs`. I'm not going to fully enumerate these because we'd be here all day, but sufficed to say, it effectively allows adding an `&` to one or both sides of an operator for primitives. Additionally, I constified the implementations for `Wrapping`, `Saturating`, and `NonZero` as well, since all of them forward to already-const-stable methods. (potentially via intrinsics, to avoid extra overhead) There are three "non-primitive" types which implement these traits, listed below. Note that I put "non-primitive" in quotes since I'm including `Wrapping`, `Saturating`, and `NonZero`, which are just wrappers over primitives. * `Duration` (arithmetic operations) * `SystemTime` (arithmetic operations) * `Ipv4Addr` (bit operations) * `Ipv6Addr` (bit operations) Additionally, because the methods on `SystemTime` needed to make these operations const were not marked const, a separate tracking issue for const-stabilising those methods is rust-lang#144517. Stuff left out of this PR: * `Assume` (this could trivially be made const, but since the docs indicate this is still under heavy design, I figured I'd leave it out) * `Instant` (this could be made const, but cannot reasonably be constructed at constant time, so, isn't useful) * `SystemTime` (will submit separate PR) * SIMD types (I'm tackling these all at once later; see rust-lang/portable-simd#467) <!-- TRIAGEBOT_START --> <!-- TRIAGEBOT_CONCERN-ISSUE_START --> > [!NOTE] > # Concerns (0 active) > > - ~~[May break Clippy](rust-lang#143949 (comment) resolved in [this comment](rust-lang#143949 (comment)) > > *Managed by ``@rustbot`—see` [help](https://forge.rust-lang.org/triagebot/concern.html) for details.* <!-- TRIAGEBOT_CONCERN-ISSUE_END --> <!-- TRIAGEBOT_END -->
Failed in rollup due to tidy: #145192 (comment) Probably needs a rebase. @bors r- |
…ibutes explicitly
Yeah, it's the bootstrap bump making the tidy rustfmt actually use the new Will rebase soon. |
59a69c7
to
bb32e31
Compare
@rustbot ready |
@bors r+ |
…nieu Constify remaining traits/impls for `const_ops` Tracking issue: rust-lang#143802 This is split into two commits for ease of reviewability: 1. Updates the `forward_ref_*` macros to accept multiple attributes (in anticipation of needing `rust_const_unstable` attributes) and also *require* attributes in these macros. Since the default attribute only helps for the initial implementations, it means it's easy to get wrong for future implementations, as shown for the saturating implementations which were incorrect before. 2. Actually constify the traits/impls. A few random other notes on the implementation specifically: * I unindented the attributes that were passed to the `forward_ref_*` macro calls because in some places rustfmt wanted them to be unindented, and in others it was allowed because they were themselves inside of macro bodies. I chose the consistent indenting even though I (personally) think it looks worse. ---- As far as the actual changes go, this constifies the following additional traits: * `Neg` * `Not` * `BitAnd` * `BitOr` * `BitXor` * `Shl` * `Shr` * `AddAssign` * `SubAssign` * `MulAssign` * `DivAssign` * `RemAssign` * `BitAndAssign` * `BitOrAssign` * `BitXorAssign` * `ShlAssign` * `ShrAssign` In terms of constified implementations of these traits, it adds the reference-forwarded versions of all the arithmetic operators, which are defined by the macros in `library/core/src/internal_macros.rs`. I'm not going to fully enumerate these because we'd be here all day, but sufficed to say, it effectively allows adding an `&` to one or both sides of an operator for primitives. Additionally, I constified the implementations for `Wrapping`, `Saturating`, and `NonZero` as well, since all of them forward to already-const-stable methods. (potentially via intrinsics, to avoid extra overhead) There are three "non-primitive" types which implement these traits, listed below. Note that I put "non-primitive" in quotes since I'm including `Wrapping`, `Saturating`, and `NonZero`, which are just wrappers over primitives. * `Duration` (arithmetic operations) * `SystemTime` (arithmetic operations) * `Ipv4Addr` (bit operations) * `Ipv6Addr` (bit operations) Additionally, because the methods on `SystemTime` needed to make these operations const were not marked const, a separate tracking issue for const-stabilising those methods is rust-lang#144517. Stuff left out of this PR: * `Assume` (this could trivially be made const, but since the docs indicate this is still under heavy design, I figured I'd leave it out) * `Instant` (this could be made const, but cannot reasonably be constructed at constant time, so, isn't useful) * `SystemTime` (will submit separate PR) * SIMD types (I'm tackling these all at once later; see rust-lang/portable-simd#467) <!-- TRIAGEBOT_START --> <!-- TRIAGEBOT_CONCERN-ISSUE_START --> > [!NOTE] > # Concerns (0 active) > > - ~~[May break Clippy](rust-lang#143949 (comment) resolved in [this comment](rust-lang#143949 (comment)) > > *Managed by ``@rustbot`—see` [help](https://forge.rust-lang.org/triagebot/concern.html) for details.* <!-- TRIAGEBOT_CONCERN-ISSUE_END --> <!-- TRIAGEBOT_END -->
Rollup of 7 pull requests Successful merges: - #143949 (Constify remaining traits/impls for `const_ops`) - #144330 (document assumptions about `Clone` and `Eq` traits) - #144350 (std: sys: io: io_slice: Add UEFI types) - #144558 (Point at the `Fn()` or `FnMut()` bound that coerced a closure, which caused a move error) - #145149 (Make config method invoke inside parse use dwn_ctx) - #145227 (Tweak spans providing type context on errors when involving macros) - #145228 (Remove unnecessary parentheses in `assert!`s) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #143949 - clarfonthey:const-arith-ops, r=Amanieu Constify remaining traits/impls for `const_ops` Tracking issue: #143802 This is split into two commits for ease of reviewability: 1. Updates the `forward_ref_*` macros to accept multiple attributes (in anticipation of needing `rust_const_unstable` attributes) and also *require* attributes in these macros. Since the default attribute only helps for the initial implementations, it means it's easy to get wrong for future implementations, as shown for the saturating implementations which were incorrect before. 2. Actually constify the traits/impls. A few random other notes on the implementation specifically: * I unindented the attributes that were passed to the `forward_ref_*` macro calls because in some places rustfmt wanted them to be unindented, and in others it was allowed because they were themselves inside of macro bodies. I chose the consistent indenting even though I (personally) think it looks worse. ---- As far as the actual changes go, this constifies the following additional traits: * `Neg` * `Not` * `BitAnd` * `BitOr` * `BitXor` * `Shl` * `Shr` * `AddAssign` * `SubAssign` * `MulAssign` * `DivAssign` * `RemAssign` * `BitAndAssign` * `BitOrAssign` * `BitXorAssign` * `ShlAssign` * `ShrAssign` In terms of constified implementations of these traits, it adds the reference-forwarded versions of all the arithmetic operators, which are defined by the macros in `library/core/src/internal_macros.rs`. I'm not going to fully enumerate these because we'd be here all day, but sufficed to say, it effectively allows adding an `&` to one or both sides of an operator for primitives. Additionally, I constified the implementations for `Wrapping`, `Saturating`, and `NonZero` as well, since all of them forward to already-const-stable methods. (potentially via intrinsics, to avoid extra overhead) There are three "non-primitive" types which implement these traits, listed below. Note that I put "non-primitive" in quotes since I'm including `Wrapping`, `Saturating`, and `NonZero`, which are just wrappers over primitives. * `Duration` (arithmetic operations) * `SystemTime` (arithmetic operations) * `Ipv4Addr` (bit operations) * `Ipv6Addr` (bit operations) Additionally, because the methods on `SystemTime` needed to make these operations const were not marked const, a separate tracking issue for const-stabilising those methods is #144517. Stuff left out of this PR: * `Assume` (this could trivially be made const, but since the docs indicate this is still under heavy design, I figured I'd leave it out) * `Instant` (this could be made const, but cannot reasonably be constructed at constant time, so, isn't useful) * `SystemTime` (will submit separate PR) * SIMD types (I'm tackling these all at once later; see rust-lang/portable-simd#467) <!-- TRIAGEBOT_START --> <!-- TRIAGEBOT_CONCERN-ISSUE_START --> > [!NOTE] > # Concerns (0 active) > > - ~~[May break Clippy](#143949 (comment) resolved in [this comment](#143949 (comment)) > > *Managed by ```@rustbot`—see`` [help](https://forge.rust-lang.org/triagebot/concern.html) for details.* <!-- TRIAGEBOT_CONCERN-ISSUE_END --> <!-- TRIAGEBOT_END -->
This PR appears to be linked to a perf regression observed at #145236 (comment). (I’m not familiar with the perf triage process, but I wanted to make sure this was explicitly noted on the relevant issue and not just the rollup.) |
Mentioned your comment on the tracking issue since this PR has been merged; no idea how perf stuff tends to be triaged, but since wg-const-eval is looking to constify as much stuff as possible, it's probably worth bringing more visibility to it there. |
Tracking issue: #143802
This is split into two commits for ease of reviewability:
forward_ref_*
macros to accept multiple attributes (in anticipation of needingrust_const_unstable
attributes) and also require attributes in these macros. Since the default attribute only helps for the initial implementations, it means it's easy to get wrong for future implementations, as shown for the saturating implementations which were incorrect before.A few random other notes on the implementation specifically:
forward_ref_*
macro calls because in some places rustfmt wanted them to be unindented, and in others it was allowed because they were themselves inside of macro bodies. I chose the consistent indenting even though I (personally) think it looks worse.As far as the actual changes go, this constifies the following additional traits:
Neg
Not
BitAnd
BitOr
BitXor
Shl
Shr
AddAssign
SubAssign
MulAssign
DivAssign
RemAssign
BitAndAssign
BitOrAssign
BitXorAssign
ShlAssign
ShrAssign
In terms of constified implementations of these traits, it adds the reference-forwarded versions of all the arithmetic operators, which are defined by the macros in
library/core/src/internal_macros.rs
. I'm not going to fully enumerate these because we'd be here all day, but sufficed to say, it effectively allows adding an&
to one or both sides of an operator for primitives.Additionally, I constified the implementations for
Wrapping
,Saturating
, andNonZero
as well, since all of them forward to already-const-stable methods. (potentially via intrinsics, to avoid extra overhead)There are three "non-primitive" types which implement these traits, listed below. Note that I put "non-primitive" in quotes since I'm including
Wrapping
,Saturating
, andNonZero
, which are just wrappers over primitives.Duration
(arithmetic operations)SystemTime
(arithmetic operations)Ipv4Addr
(bit operations)Ipv6Addr
(bit operations)Additionally, because the methods on
SystemTime
needed to make these operations const were not marked const, a separate tracking issue for const-stabilising those methods is #144517.Stuff left out of this PR:
Assume
(this could trivially be made const, but since the docs indicate this is still under heavy design, I figured I'd leave it out)Instant
(this could be made const, but cannot reasonably be constructed at constant time, so, isn't useful)SystemTime
(will submit separate PR)Note
Concerns (0 active)
May break Clippyresolved in this commentManaged by
@rustbot
—see help for details.