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

Consolidate all associated items on the NonZero integer types into a single impl block per type #118665

Merged
merged 18 commits into from
Jan 19, 2024

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Dec 6, 2023

Before:

#[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:

#[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: #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 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.

impl i8 {
int_impl! {
Self = i8,
ActualT = i8,
UnsignedT = u8,
BITS = 8,
BITS_MINUS_ONE = 7,
Min = -128,
Max = 127,
rot = 2,
rot_op = "-0x7e",
rot_result = "0xa",
swap_op = "0x12",
swapped = "0x12",
reversed = "0x48",
le_bytes = "[0x12]",
be_bytes = "[0x12]",
to_xe_bytes_doc = "",
from_xe_bytes_doc = "",
bound_condition = "",
}
}

Best reviewed one commit at a time.

@rustbot
Copy link
Collaborator

rustbot commented Dec 6, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 6, 2023
@rust-log-analyzer

This comment has been minimized.

@dtolnay dtolnay force-pushed the signedness branch 2 times, most recently from 4a13b47 to 66eae65 Compare December 8, 2023 06:10
Copy link
Member

@Noratrieb Noratrieb left a 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.

@bors
Copy link
Contributor

bors commented Jan 12, 2024

☔ 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
@dtolnay
Copy link
Member Author

dtolnay commented Jan 14, 2024

Copy link
Member

@Noratrieb Noratrieb left a 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

@Noratrieb
Copy link
Member

Noratrieb commented Jan 15, 2024

well, diffed the two files before/after anyways, looks fine.

diff
115a116,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())

@Noratrieb
Copy link
Member

I guess I'm supposed to do this now :)
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 18, 2024

📌 Commit cdee1fe has been approved by Nilstrieb

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2024
…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
@bors bors merged commit 122b3f9 into rust-lang:master Jan 19, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2024
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&times; 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.
@dtolnay dtolnay deleted the signedness branch January 21, 2024 04:23
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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants