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

core::num::NonZero<N> type alias #88990

Closed
wants to merge 2 commits into from
Closed

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Sep 15, 2021

This PR implements NonZero<N> as I proposed in #82363 (comment), and deletes unstable NonZero_c_*.

NonZero<N> is a generic type alias for the non-zero type that corresponds to each integer primitive.

This type alias simply provides an alternative spelling of the various NonZero structs found in core::num. For example NonZero<u16> refers to the struct core::num::NonZeroU16.

Note: this is not intended for writing code that is generic over multiple types of non-zero integers. The relevant trait bound you would need for that is not exposed. The only thing this is, is an alternative spelling.

Example

Where this comes in handy is if the primitive types themselves are being used via a type alias, such as std::os::raw::c_char. The c_char type refers to either i8 or u8 depending on whether the platform's C character type is signed or unsigned, and without NonZero<N> there is not a straightforward way to name either NonZeroI8 or NonZeroU8 depending on which one a non-zero c_char corresponds to.

#![feature(nonzero_generic_type_alias)]

use std::num::NonZero;
use std::os::raw::c_char;

// A separator that we're planning to use with nul-terminated C strings,
// where \0 would be nonsensical.
pub struct Separator {
    ch: NonZero<c_char>,
}

impl Separator {
    pub fn new(ch: c_char) -> Option<Self> {
        NonZero::new(ch).map(|ch| Separator { ch })
    }
}

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2021
@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 15, 2021
@rust-log-analyzer

This comment has been minimized.

@kennytm
Copy link
Member

kennytm commented Sep 16, 2021

heh. so we have come to a full circle of introducing NonZero<T> again. why was rust-lang/rfcs#2307 accepted in the first place 🤷

@dtolnay
Copy link
Member Author

dtolnay commented Sep 16, 2021

Shrug, this is not at all the NonZero from #27730. Every single benefit from rust-lang/rfcs#2307 still applies. I can't tell if you are just joking but this is forward progress, not full circle.

@kennytm
Copy link
Member

kennytm commented Sep 17, 2021

Well both?

Surely the primary purpose of of rust-lang/rfcs#2307 is to remove the public Zeroable trait. But now the role of that trait comes back as the private IntegerPrimitive trait, which API-wise the trait's privacy is the only difference compared with #27730. The private trait negates the entire Motivation of introducing the 12 distinct types since users can't impl NonZero<CustomType> at all:

One problem of the current API is that it is unclear what happens or what should happen to NonZero<T> or Option<NonZero<T>> when T is some type other than a raw pointer or a primitive integer. In particular, crates outside of std can implement Zeroable for their arbitrary types since it is a public trait.

To avoid this question entirely, this RFC proposes replacing the generic type and trait with twelve concrete types in std::num, one for each primitive integer type. This is similar to the existing atomic integer types like std::sync::atomic::AtomicU32.

So I think it is only natural to take one step further (backward?) and make it

#[repr(transparent)]
#[rustc_layout_scalar_valid_range_start(1)]
#[rustc_nonnull_optimization_guaranteed]
pub struct NonZero<T: IntegerPrimitive>(T);

// ... then most impl can be freed from the macros ...

// stability compatibility ↓

pub type NonZeroU8 = NonZero<u8>;
...
pub type NonZeroI128 = NonZero<i128>;

Edit: I'm moving this to #82363 to not clutter the PR itself.

@kennytm
Copy link
Member

kennytm commented Sep 17, 2021

i'm going to wait till #82363's current FCP is canceled since this PR removes the NonZero_c_* types.

@kennytm kennytm added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2021
@bors
Copy link
Contributor

bors commented Oct 4, 2021

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

@dtolnay
Copy link
Member Author

dtolnay commented Oct 5, 2021

Kenny's writeup in #82363 (comment) is compelling. Gonna close this in favor of struct NonZero 🙂

@dtolnay dtolnay closed this Oct 5, 2021
@dtolnay dtolnay deleted the nonzero branch October 5, 2021 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants