Skip to content

Add const-friendly NonZero::new_unwrap#602

Merged
tarcieri merged 4 commits into
RustCrypto:masterfrom
dvdplm:dp-non-zero-new_unchecked
Jul 26, 2024
Merged

Add const-friendly NonZero::new_unwrap#602
tarcieri merged 4 commits into
RustCrypto:masterfrom
dvdplm:dp-non-zero-new_unchecked

Conversation

@dvdplm

@dvdplm dvdplm commented Jun 3, 2024

Copy link
Copy Markdown
Contributor

Makes it so users can have NonZero constants.

Makes it so users can have `NonZero` constants.
@dvdplm

dvdplm commented Jun 3, 2024

Copy link
Copy Markdown
Contributor Author

There used to be a const_new for this purpose but it relied on the custom CtChoice machinery that is gone now (for good reason!). Maybe this can be a stop-gap solution until we get to a better place with const support?

@tarcieri

tarcieri commented Jun 3, 2024

Copy link
Copy Markdown
Member

The constructor is also what's checking the T: Zero bound. This approach with absolutely no checks allows you to make NonZero<String> and all sorts of other nonsense type combinations.

Perhaps instead until const impl is stable it could be defined directly on concrete types like e.g. NonZero<Uint<LIMBS>> which would also enable checking for zero and panicking, rather than allowing for construction of a numerically invalid value.

@dvdplm

dvdplm commented Jun 3, 2024

Copy link
Copy Markdown
Contributor Author

The constructor is also what's checking the T: Zero bound. This approach with absolutely no checks allows you to make NonZero and all sorts of other nonsense type combinations.

I can add the Zero bound of course, but that's a bit of a "false safety" no?

@dvdplm

dvdplm commented Jun 3, 2024

Copy link
Copy Markdown
Contributor Author

Perhaps instead until const impl is stable it could be defined directly on concrete types like e.g. NonZero<Uint<LIMBS>> which would also enable checking for zero and panicking, rather than allowing for construction of a numerically invalid value.

You mean bring back a variant of what 0.5.5 had?

impl<const LIMBS: usize> NonZero<Uint<LIMBS>> {
    /// Creates a new non-zero integer in a const context.
    /// The second return value is `FALSE` if `n` is zero, `TRUE` otherwise.
    pub const fn const_new(n: Uint<LIMBS>) -> (Self, CtChoice) {
        (Self(n), n.ct_is_nonzero())
    }
}

…but without CtChoice?

@tarcieri

tarcieri commented Jun 3, 2024

Copy link
Copy Markdown
Member

Yes, I forget offhand why we got rid of that, but for defining constants a const constructor that panics is probably preferable until e.g. .unwrap()/.expect() can work in const contexts

@dvdplm

dvdplm commented Jun 3, 2024

Copy link
Copy Markdown
Contributor Author

I found a workaround for my concrete issue using to_nz() and expect (both const):

trait Beef {
    const MOO: NonZero<Uint<16>>
}

struct MyBeef;
impl Beef for MyBeef {
    const MOO: U512::from_be_hex("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141")
        .to_nz()
        .expect("Valid by construction");
}

I had a stab at making a panicking const_new, but ran into issues. I'll have another go later.

@dvdplm

dvdplm commented Jun 4, 2024

Copy link
Copy Markdown
Contributor Author

@tarcieri PTAL at cf64867 – is that what you had in mind here? :)

@tarcieri

tarcieri commented Jun 4, 2024

Copy link
Copy Markdown
Member

Sure, that works. You could also implement it with .to_nz().expect as per above.

@dvdplm dvdplm changed the title Add a new_unchecked const method to NonZero Add a const_new method to NonZero Jun 5, 2024
@dvdplm

dvdplm commented Jul 25, 2024

Copy link
Copy Markdown
Contributor Author

Is there anything left to do here before merging?

Comment thread src/non_zero.rs Outdated
Comment thread src/non_zero.rs Outdated
Co-authored-by: Tony Arcieri <bascule@gmail.com>
@tarcieri tarcieri merged commit 295d222 into RustCrypto:master Jul 26, 2024
@tarcieri tarcieri changed the title Add a const_new method to NonZero Add const-friendly NonZero::new_unwrap Aug 1, 2024
@tarcieri tarcieri mentioned this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants