-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Consolidate all associated items on the NonZero integer types into a single impl block per type #118665
Conversation
r? @m-ou-se (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
4a13b47
to
66eae65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtolnay asked me to take a look: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/nils'.20reviews/near/411787682
I looked at all commits and they were all pretty simple in isolation and I think they all look correct to me. The change also looks reasonable for the purpose of unifying the impl blocks.
☔ The latest upstream changes (presumably #119452) made this pull request unmergeable. Please resolve the merge conflicts. |
This way all the other macros defined in this module, such as nonzero_leading_trailing_zeros, are available to call within the expansion of nonzero_integers. (Macros defined by macro_rules cannot be called from the same module above the location of the macro_rules.) In this commit the ability to call things like nonzero_leading_trailing_zeros is not immediately used, but later commits in this stack will be consolidating the entire API of NonZeroT to be generated through nonzero_integers, and will need to make use of some of the other macros to do that.
Later in this stack, as the nonzero_integers macro is going to be responsible for producing a larger fraction of the API for the NonZero integer types, it will need to receive a number of additional arguments beyond the ones currently seen here. Additional arguments, especially named arguments across multiple lines, will turn out clearer if everything in one macro call is for the same NonZero type. This commit adopts a similar arrangement to what we do for generating the API of the integer primitives (`impl u8` etc), which also generate a single type's API per top-level macro call, rather than generating all 12 impl blocks for the 12 types from one macro call.
…mpls The `key = $value` style will be beneficial as we introduce some more macro arguments here in later commits.
tidy error: /git/rust/library/core/src/num/nonzero.rs:67: malformed stability attribute: missing `feature` key tidy error: /git/rust/library/core/src/num/nonzero.rs:82: malformed stability attribute: missing `feature` key tidy error: /git/rust/library/core/src/num/nonzero.rs:98: malformed stability attribute: missing the `since` key tidy error: /git/rust/library/core/src/num/nonzero.rs:112: malformed stability attribute: missing `feature` key tidy error: /git/rust/library/core/src/num/nonzero.rs:450: malformed stability attribute: missing `feature` key some tidy checks failed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't review the diff in detail again, but it should be fine
well, diffed the two files before/after anyways, looks fine. diff115a116,127
> // FIXME: Remove this after LLVM supports `!range` metadata for function
> // arguments https://github.com/llvm/llvm-project/issues/76628
> //
> // Rustc can set range metadata only if it loads `self` from
> // memory somewhere. If the value of `self` was from by-value argument
> // of some not-inlined function, LLVM don't have range metadata
> // to understand that the value cannot be zero.
>
> // SAFETY: It is an invariant of this type.
> unsafe {
> intrinsics::assume(self.0 != 0);
> }
153c165
< unsafe { intrinsics::ctlz_nonzero(self.0 as $UnsignedPrimitive) as u32 }
---
> unsafe { intrinsics::ctlz_nonzero(self.get() as $UnsignedPrimitive) as u32 }
177c189
< unsafe { intrinsics::cttz_nonzero(self.0 as $UnsignedPrimitive) as u32 }
---
> unsafe { intrinsics::cttz_nonzero(self.get() as $UnsignedPrimitive) as u32 }
394c406,408
< nonzero.0
---
> // Call nonzero to keep information range information
> // from get method.
> nonzero.get()
760c774
< super::int_log10::$Int(self.0)
---
> super::int_log10::$Int(self.get()) |
I guess I'm supposed to do this now :) |
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#118665 (Consolidate all associated items on the NonZero integer types into a single impl block per type) - rust-lang#118798 (Use AtomicU8 instead of AtomicUsize in backtrace.rs) - rust-lang#119062 (Deny braced macro invocations in let-else) - rust-lang#119138 (Docs: Use non-SeqCst in module example of atomics) - rust-lang#119907 (Update `fn()` trait implementation docs) - rust-lang#120083 (Warn when not having a profiler runtime means that coverage tests won't be run/blessed) - rust-lang#120107 (dead_code treats #[repr(transparent)] the same as #[repr(C)]) - rust-lang#120110 (Update documentation for Vec::into_boxed_slice to be more clear about excess capacity) - rust-lang#120113 (Remove myself from review rotation) - rust-lang#120118 (Fix typo in documentation in base.rs) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#118665 (Consolidate all associated items on the NonZero integer types into a single impl block per type) - rust-lang#118798 (Use AtomicU8 instead of AtomicUsize in backtrace.rs) - rust-lang#119062 (Deny braced macro invocations in let-else) - rust-lang#119138 (Docs: Use non-SeqCst in module example of atomics) - rust-lang#119907 (Update `fn()` trait implementation docs) - rust-lang#120083 (Warn when not having a profiler runtime means that coverage tests won't be run/blessed) - rust-lang#120107 (dead_code treats #[repr(transparent)] the same as #[repr(C)]) - rust-lang#120110 (Update documentation for Vec::into_boxed_slice to be more clear about excess capacity) - rust-lang#120113 (Remove myself from review rotation) - rust-lang#120118 (Fix typo in documentation in base.rs) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#118665 - dtolnay:signedness, r=Nilstrieb Consolidate all associated items on the NonZero integer types into a single impl block per type **Before:** ```rust #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] pub struct NonZeroI8(i8); impl NonZeroI8 { pub const fn new(n: i8) -> Option<Self> ... pub const fn get(self) -> i8 ... } impl NonZeroI8 { pub const fn leading_zeros(self) -> u32 ... pub const fn trailing_zeros(self) -> u32 ... } impl NonZeroI8 { pub const fn abs(self) -> NonZeroI8 ... } ... ``` **After:** ```rust #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] pub struct NonZeroI8(i8); impl NonZeroI8 { pub const fn new(n: i8) -> Option<Self> ... pub const fn get(self) -> i8 ... pub const fn leading_zeros(self) -> u32 ... pub const fn trailing_zeros(self) -> u32 ... pub const fn abs(self) -> NonZeroI8 ... ... } ``` Having 6-7 different impl blocks per type is not such a problem in today's implementation, but becomes awful upon the switch to a generic `NonZero<T>` type (context: rust-lang#82363 (comment)). In the implementation from rust-lang#100428, there end up being **67** impl blocks on that type. <img src="https://github.com/rust-lang/rust/assets/1940490/5b68bd6f-8a36-4922-baa3-348e30dbfcc1" width="200"><img src="https://github.com/rust-lang/rust/assets/1940490/2cfec71e-c2cd-4361-a542-487f13f435d9" width="200"><img src="https://github.com/rust-lang/rust/assets/1940490/2fe00337-7307-405d-9036-6fe1e58b2627" width="200"> Without the refactor to a single impl block first, introducing `NonZero<T>` would be a usability regression compared to today's separate pages per type. With all those blocks expanded, Ctrl+F is obnoxious because you need to skip 12× past every match you don't care about. With all the blocks collapsed, Ctrl+F is useless. Getting to a state in which exactly one type's (e.g. `NonZero<u32>`) impl blocks are expanded while the rest are collapsed is annoying. After this refactor to a single impl block, we can move forward with making `NonZero<T>` a generic struct whose docs all go on the same rustdoc page. The rustdoc will have 12 impl blocks, one per choice of `T` supported by the standard library. The reader can expand a single one of those impl blocks e.g. `NonZero<u32>` to understand the entire API of that type. Note that moving the API into a generic `impl<T> NonZero<T> { ... }` is not going to be an option until after `NonZero<T>` has been stabilized, which may be months or years after its introduction. During the period while generic `NonZero` is unstable, it will be extra important to offer good documentation on all methods demonstrating the API being used through the stable aliases such as `NonZeroI8`. This PR follows a `key = $value` syntax for the macros which is similar to the macros we already use for producing a single large impl block on the integer primitives. https://github.com/rust-lang/rust/blob/1dd4db50620fb38a6382c22456a96ed7cddeff83/library/core/src/num/mod.rs#L288-L309 Best reviewed one commit at a time.
Before:
After:
Having 6-7 different impl blocks per type is not such a problem in today's implementation, but becomes awful upon the switch to a generic
NonZero<T>
type (context: #82363 (comment)).In the implementation from #100428, there end up being 67 impl blocks on that type.
Without the refactor to a single impl block first, introducing
NonZero<T>
would be a usability regression compared to today's separate pages per type. With all those blocks expanded, Ctrl+F is obnoxious because you need to skip 12× past every match you don't care about. With all the blocks collapsed, Ctrl+F is useless. Getting to a state in which exactly one type's (e.g.NonZero<u32>
) impl blocks are expanded while the rest are collapsed is annoying.After this refactor to a single impl block, we can move forward with making
NonZero<T>
a generic struct whose docs all go on the same rustdoc page. The rustdoc will have 12 impl blocks, one per choice ofT
supported by the standard library. The reader can expand a single one of those impl blocks e.g.NonZero<u32>
to understand the entire API of that type.Note that moving the API into a generic
impl<T> NonZero<T> { ... }
is not going to be an option until afterNonZero<T>
has been stabilized, which may be months or years after its introduction. During the period while genericNonZero
is unstable, it will be extra important to offer good documentation on all methods demonstrating the API being used through the stable aliases such asNonZeroI8
.This PR follows a
key = $value
syntax for the macros which is similar to the macros we already use for producing a single large impl block on the integer primitives.rust/library/core/src/num/mod.rs
Lines 288 to 309 in 1dd4db5
Best reviewed one commit at a time.