Implement Bounded, ToPrimitive, NumCast, FromPrimitive for NonZero* #334

acrrd · 2024-08-29T20:41:13Z

I have a use case where I convert to and from primitive, and I need to involve NonZero* types as well.
Currently, I have a function that uses these traits for primitive types, and I have additional functions for each NonZero* that I need. It would be nice to get rid of them and have only one.

I can create on PR per trait implementation if it's preferred. This PR has one commit per trait implementation.

foolishell · 2024-09-13T10:21:42Z

@acrrd
Thank you so much for this pull request. If possible, could you also add support for AsPrimitive?

tarcieri · 2024-09-17T22:05:18Z

Curious about https://doc.rust-lang.org/stable/core/num/struct.NonZero.html as well (might need an MSRV-preserving feature-gate)

acrrd · 2024-09-17T22:17:50Z

@acrrd Thank you so much for this pull request. If possible, could you also add support for AsPrimitive?

done

acrrd · 2024-09-17T22:24:34Z

Curious about https://doc.rust-lang.org/stable/core/num/struct.NonZero.html as well (might need an MSRV-preserving feature-gate)

This needs nightly, NonZero is only usable with types that implement ZeroablePrimitive. This trait is usable only in nightly.
When stabilized it will reduce the needed code to implement NonZero, but now is not usable.

tarcieri · 2024-09-17T22:39:54Z

@acrrd though the ZeroablePrimitive trait itself is unstable, there are impls for it for the core integer types already which are usable on stable Rust:

https://doc.rust-lang.org/stable/core/num/trait.ZeroablePrimitive.html#implementors

Quite a bit of the functionality is already stable, as is noted in the feature gates in the rustdoc. Here's a small example:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9c4126921b16b781588732fd26d050a4

cuviper · 2024-09-17T22:43:33Z

The old types are just aliases now, literally type NonZeroI32 = NonZero<i32>; etc. So implementing traits here on those aliases has exactly the same effect as implementing NonZero<T> for specific T.

cuviper · 2024-09-17T22:50:59Z

I'm torn on AsPrimitive -- right now that is only implement for types where you could literally write x as T in non-generic settings. It's not sealed though, and the docs do mention how newtypes should behave too.

So if we add that, shouldn't we also do it for Wrapping? (and Saturating, if we get to that at all)

acrrd · 2024-09-18T16:00:28Z

I'm torn on AsPrimitive -- right now that is only implement for types where you could literally write x as T in non-generic settings. It's not sealed though, and the docs do mention how newtypes should behave too.

It was requested in #335, they have a use case for it. Let me know if you want to keep it.

So if we add that, shouldn't we also do it for Wrapping? (and Saturating, if we get to that at all)

We can't. The trait requires Add,Sub, etc., which are not implemented for NonZero*. This is related to #274.

cuviper · 2024-09-18T17:48:40Z

I'm torn on AsPrimitive -- [...]

It was requested in #335, they have a use case for it. Let me know if you want to keep it.

I'm going to respond there, because I think that use is also problematic, even aside from NonZero. Let's drop it here for now.

So if we add that, shouldn't we also do it for Wrapping? (and Saturating, if we get to that at all)

We can't. The trait requires Add,Sub, etc., which are not implemented for NonZero*. This is related to #274.

I wasn't talking about any other traits, only impl<T, U> AsPrimitive<T> for core::num::Wrapping<U>. But that deserves a separate PR, if we even decide that's worthwhile.

acrrd · 2024-09-18T18:14:45Z

I'm going to respond there, because I think that use is also problematic, even aside from NonZero. Let's drop it here for now.

done

I wasn't talking about any other traits, only impl<T, U> AsPrimitive<T> for core::num::Wrapping<U>. But that deserves a separate PR, if we even decide that's worthwhile.

I see it now, sorry

marshrayms · 2024-09-26T18:23:40Z

To me, a big part of the value of add-on crates like the rust-num collection is that they can help make up where the base language falls short.

I'm torn on AsPrimitive -- right now that is only implement for types where you could literally write x as T in non-generic settings. It's not sealed though, and the docs do mention how newtypes should behave too.

For example, the concept you mentioned of "types where you could literally write x as T" is not expressible using any combination of traits from the standard library.

So if we add that, shouldn't we also do it for Wrapping? (and Saturating, if we get to that at all)

I think the difference is that NonZero* is a newtype which expresses an inherent property of the value itself. They are useful as type system constraints.

Whereas Wrapping and Saturating express nothing about the set of values they represent, they are simply used to select different sets of common functions via operator overloading. Purely syntactic convenience.

cuviper · 2024-09-26T18:44:03Z

To me, a big part of the value of add-on crates like the rust-num collection is that they can help make up where the base language falls short.

I'm torn on AsPrimitive -- right now that is only implement for types where you could literally write x as T in non-generic settings. It's not sealed though, and the docs do mention how newtypes should behave too.

For example, the concept you mentioned of "types where you could literally write x as T" is not expressible using any combination of traits from the standard library.

This seems consistent to me -- AsPrimitive is currently filling that gap in the base language by making it expressible as a trait, though you only get .as_() instead of a real as in that generic code. We can choose to extend the domain of that trait as well, and maybe we should, but I think we should do that separately.

Anyway, let me give this PR an actual review...

cuviper · 2024-09-26T18:57:41Z

src/bounds.rs

+        const $i: $t = {
+            let arr = [$v];
+            let idx = if $v != 0 { 0 } else { 1 };
+
+            unsafe { <$t>::new_unchecked(arr[idx]) }
+        };


This can be done safely -- NonZero::new is const since 1.47, and const panic! since 1.57. (Our MSRV is 1.60.)

Suggested change

const $i: $t = {

let arr = [$v];

let idx = if $v != 0 { 0 } else { 1 };

unsafe { <$t>::new_unchecked(arr[idx]) }

};

const $i: $t = match <$t>::new($v) {

Some(nz) => nz,

None => panic!("bad nonzero bound!"),

};

And once our MSRV is 1.70 we should use the inherent consts, so please leave a comment for that.

Nice, done.

cuviper · 2024-09-26T19:03:18Z

src/cast.rs

+macro_rules! impl_to_primitive_nonzero_to_method {
+    ($SrcT:ident : $( $(#[$cfg:meta])* fn $method:ident -> $DstT:ident ; )*) => {$(
+        #[inline]
+        $(#[$cfg])*


I guess you copied the cfg from the other macros, but it's vestigial, from the days when i128 was conditional. We definitely shouldn't propagate that, but could you clean up the others too?

I wasn't sure what it was so I just kept it. Removed here and from other macros

cuviper · 2024-09-26T19:05:21Z

src/cast.rs

+
+            #[inline]
+            fn to_f32(&self) -> Option<f32> {
+                Some(self.get() as f32)


Why isn't this forwarding to the inner to_f32 as well?

Not sure, I think I copied from the other macro and didn't notice here I could put everything together.

cuviper · 2024-09-26T19:08:13Z

src/cast.rs

@@ -572,6 +631,85 @@ impl_from_primitive!(u128, to_u128);
 impl_from_primitive!(f32, to_f32);
 impl_from_primitive!(f64, to_f64);

+macro_rules! impl_from_primitive_nonzero {
+    ($T:ty, $to_ty:ident) => {
+        #[allow(deprecated)]


I think this is also vestigial, way back from commit 03db5c9 when it was copied from its deprecated state in the standard library.

acrrd force-pushed the support_nonzero branch 4 times, most recently from 03bb963 to eee8ac0 Compare August 30, 2024 11:11

acrrd added 4 commits September 18, 2024 00:00

Implement Bounded for NonZero*

22ce2dc

Implement ToPrimitive for NonZero*

2b9af3d

Implement NumCast for NonZero*

397037d

Implement FromPrimitive for NonZero*

acb1a26

acrrd force-pushed the support_nonzero branch from eee8ac0 to 7f6c0e5 Compare September 17, 2024 22:03

cuviper mentioned this pull request Sep 18, 2024

Feature Request: Impl AsPrimitive for NonZero #335

Closed

acrrd force-pushed the support_nonzero branch from 7f6c0e5 to acb1a26 Compare September 18, 2024 18:13

cuviper requested changes Sep 26, 2024

View reviewed changes

acrrd added 4 commits September 30, 2024 15:00

Remove cfg from macros

542884d

Use to_f* in ToPrimitive impl

d4e5b0a

Remove #[allow(deprecated)]

b43a252

Remove unsafe from min/max bounds

2ce0fb6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Bounded, ToPrimitive, NumCast, FromPrimitive for NonZero* #334

Implement Bounded, ToPrimitive, NumCast, FromPrimitive for NonZero* #334

acrrd commented Aug 29, 2024 •

edited

Loading

foolishell commented Sep 13, 2024

tarcieri commented Sep 17, 2024

acrrd commented Sep 17, 2024

acrrd commented Sep 17, 2024

tarcieri commented Sep 17, 2024

cuviper commented Sep 17, 2024

cuviper commented Sep 17, 2024

acrrd commented Sep 18, 2024 •

edited

Loading

cuviper commented Sep 18, 2024

acrrd commented Sep 18, 2024

marshrayms commented Sep 26, 2024 •

edited

Loading

cuviper commented Sep 26, 2024

cuviper Sep 26, 2024

acrrd Sep 30, 2024

cuviper Sep 26, 2024

acrrd Sep 30, 2024

cuviper Sep 26, 2024

acrrd Sep 30, 2024

cuviper Sep 26, 2024

acrrd Sep 30, 2024

Implement Bounded, ToPrimitive, NumCast, FromPrimitive for NonZero* #334

Are you sure you want to change the base?

Implement Bounded, ToPrimitive, NumCast, FromPrimitive for NonZero* #334

Conversation

acrrd commented Aug 29, 2024 • edited Loading

foolishell commented Sep 13, 2024

tarcieri commented Sep 17, 2024

acrrd commented Sep 17, 2024

acrrd commented Sep 17, 2024

tarcieri commented Sep 17, 2024

cuviper commented Sep 17, 2024

cuviper commented Sep 17, 2024

acrrd commented Sep 18, 2024 • edited Loading

cuviper commented Sep 18, 2024

acrrd commented Sep 18, 2024

marshrayms commented Sep 26, 2024 • edited Loading

cuviper commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrrd commented Aug 29, 2024 •

edited

Loading

acrrd commented Sep 18, 2024 •

edited

Loading

marshrayms commented Sep 26, 2024 •

edited

Loading