-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implemented MIN and MAX constants for the non-zero integer types.#89065 #89077
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. Please see the contribution instructions for more information. |
library/core/src/num/nonzero.rs
Outdated
( $( $MinVal:expr , $Ty: ident($Int: ty); )+ ) => { | ||
$( | ||
impl $Ty { | ||
#[unstable(feature = "nonzero_max", issue = "89065")] |
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.
Don't see a reason to use two separate feature gates for min and max.
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.
Agreed. 591b89d
library/core/src/num/nonzero.rs
Outdated
// SAFETY: Since the MAX value, for any supported integer type, is greater than 0, the MAX will always be non-zero. | ||
pub const MAX : $Ty = unsafe { $Ty::new_unchecked(<$Int>::MAX) }; |
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.
// SAFETY: Since the MAX value, for any supported integer type, is greater than 0, the MAX will always be non-zero. | |
pub const MAX : $Ty = unsafe { $Ty::new_unchecked(<$Int>::MAX) }; | |
pub const MAX : $Ty = $Ty::new(<$Int>::MAX).unwrap(); |
Since we're in a const context this is guaranteed to get evaluated (and potentially fail if you pass in a zero after all) at compile time. No need for unsafe.
Same for the min case below. Unfortunately unwrap_or
isn't const yet or we could get rid of the $MinVal
input entirely.
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.
I agree! I really wanted to get rid of that $MinVal, but couldn't find a clean way. Fixed the unsafe.
591b89d
library/core/src/num/nonzero.rs
Outdated
// SAFETY: In the signed case, the minimum integer is negative, and therefore non-zero. | ||
// SAFETY: In the unsignedd case, we use one, which is non-zero. | ||
pub const MIN : $Ty = unsafe { $Ty::new_unchecked($MinVal)}; | ||
} |
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.
Indentation looks off here. Did the formatter really demand it like that?
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.
It's a macro, it should be formatted manually.
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.
Do you have specific advice on how to format it? I noticed that macros needed manual formatting. Wasn't sure if I had it right though @ibraheemdev .
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.
This closing brace, and the analogous one in nonzero_constants_signed
should be unindented by one level = four spaces. The rest looks fine.
library/core/src/num/nonzero.rs
Outdated
macro_rules! nonzero_min_max { | ||
( $( $MinVal:expr , $Ty: ident($Int: ty); )+ ) => { | ||
$( | ||
impl $Ty { | ||
#[unstable(feature = "nonzero_min_max", issue = "89065")] | ||
#[doc = concat!("The maximum value for a`", stringify!($Ty), "` is the same as `", stringify!($Int), "`")] | ||
#[doc = concat!("assert_eq!(", stringify!($Ty), "::MAX, ", stringify!($Int), "::MAX);")] | ||
pub const MAX : $Ty = $Ty::new(<$Int>::MAX).unwrap() ; | ||
#[unstable(feature = "nonzero_min_max", issue = "89065")] | ||
#[doc = concat!("The minimum value for a`", stringify!($Ty), "`.")] | ||
/// # Examples | ||
#[doc = concat!("assert_eq!(", stringify!($Ty), "::MIN, ", stringify!($MinVal), ";")] | ||
pub const MIN : $Ty = $Ty::new($MinVal).unwrap(); | ||
} | ||
)+ | ||
} | ||
} | ||
|
||
nonzero_min_max! { | ||
1 , NonZeroU8(u8); | ||
1 , NonZeroU16(u16); | ||
1 , NonZeroU32(u32); | ||
1 , NonZeroU64(u64); | ||
1 , NonZeroU128(u128); | ||
1 , NonZeroUsize(usize); | ||
i8::MIN , NonZeroI8(i8); | ||
i16::MIN , NonZeroI16(i16); | ||
i32::MIN , NonZeroI32(i32); | ||
i64::MIN , NonZeroI64(i64); | ||
i128::MIN , NonZeroI128(i128); | ||
isize::MIN , NonZeroIsize(isize); | ||
} |
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.
macro_rules! nonzero_min_max { | |
( $( $MinVal:expr , $Ty: ident($Int: ty); )+ ) => { | |
$( | |
impl $Ty { | |
#[unstable(feature = "nonzero_min_max", issue = "89065")] | |
#[doc = concat!("The maximum value for a`", stringify!($Ty), "` is the same as `", stringify!($Int), "`")] | |
#[doc = concat!("assert_eq!(", stringify!($Ty), "::MAX, ", stringify!($Int), "::MAX);")] | |
pub const MAX : $Ty = $Ty::new(<$Int>::MAX).unwrap() ; | |
#[unstable(feature = "nonzero_min_max", issue = "89065")] | |
#[doc = concat!("The minimum value for a`", stringify!($Ty), "`.")] | |
/// # Examples | |
#[doc = concat!("assert_eq!(", stringify!($Ty), "::MIN, ", stringify!($MinVal), ";")] | |
pub const MIN : $Ty = $Ty::new($MinVal).unwrap(); | |
} | |
)+ | |
} | |
} | |
nonzero_min_max! { | |
1 , NonZeroU8(u8); | |
1 , NonZeroU16(u16); | |
1 , NonZeroU32(u32); | |
1 , NonZeroU64(u64); | |
1 , NonZeroU128(u128); | |
1 , NonZeroUsize(usize); | |
i8::MIN , NonZeroI8(i8); | |
i16::MIN , NonZeroI16(i16); | |
i32::MIN , NonZeroI32(i32); | |
i64::MIN , NonZeroI64(i64); | |
i128::MIN , NonZeroI128(i128); | |
isize::MIN , NonZeroIsize(isize); | |
} | |
macro_rules! nonzero_min_max { | |
( $( $Ty:ident($Int:ty) => $min_val:expr, $max_val:expr )+ ) => { | |
$( | |
impl $Ty { | |
#[unstable(feature = "nonzero_min_max", issue = "89065")] | |
#[doc = concat!("The maximum value for a`", stringify!($Ty), "` is the same as `", stringify!($Int), "`")] | |
#[doc = concat!("assert_eq!(", stringify!($Ty), "::MAX, ", stringify!($Int), "::MAX);")] | |
pub const MAX : $Ty = $Ty::new($max_val).unwrap() ; | |
#[unstable(feature = "nonzero_min_max", issue = "89065")] | |
#[doc = concat!("The minimum value for a`", stringify!($Ty), "`.")] | |
/// # Examples | |
#[doc = concat!("assert_eq!(", stringify!($Ty), "::MIN, ", stringify!($MinVal), ";")] | |
pub const MIN: $Ty = $Ty::new($min_val).unwrap(); | |
} | |
)+ | |
} | |
} | |
nonzero_min_max! { | |
NonZeroU8 => 1, u8::MAX; | |
NonZeroU16 => 1, u16::MAX; | |
NonZeroU32 => 1, u32::MAX; | |
NonZeroU64 => 1, u64::MAX; | |
NonZeroU128 => 1, u128::MAX; | |
NonZeroUsize => 1, usize::MAX; | |
NonZeroI8 => i8::MIN, i8::MAX; | |
NonZeroI16 => i16::MIN, i16::MAX; | |
NonZeroI32 => i32::MIN, i32::MAX; | |
NonZeroI64 => i64::MIN, i64::MAX; | |
NonZeroI128 => i128::MIN, i128::MAX; | |
NonZeroIsize => isize::MIN, isize::MAX; | |
} |
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.
Formatting is up to you, but I'd probably do something like this.
Mirroring part of a comment from @kpreid so it doesn't get forgotten about:
I agree that this hole should be called out explicitly in the documentation. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I think this is resolved. |
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.
I agree that the zero case has been mentioned adequately, but since I looked, here's some minor suggestions (all to do with the documentation or formatting).
#[unstable(feature = "nonzero_min_max", issue = "89065")] | ||
#[doc = concat!("The maximum value for a`", stringify!($Ty), "` is the same as `", stringify!($Int), "`")] | ||
/// # Examples | ||
#[doc = concat!("assert_eq!(", stringify!($Ty), "::MAX, ", stringify!($Int), "::MAX);")] |
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.
I think this and the below example need a ``` ... ``` code block, for formatting (though they're not very interesting as doctests).
I think including some blank lines as done above for nonzero_unsigned_signed_operations
(such as ``` above and below the Examples header, and a normal blank line between the MAX and MIN definitions) but before would also make the macro code more readable even if it doesn't affect the documentation output.
library/core/src/num/nonzero.rs
Outdated
$( | ||
impl $Ty { | ||
#[unstable(feature = "nonzero_min_max", issue = "89065")] | ||
#[doc = concat!("The maximum value for a`", stringify!($Ty), "` is the same as `", stringify!($Int), "`")] |
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.
In for a`",
there should be a space between a
and `
, and the sentence needs a period at the end.
library/core/src/num/nonzero.rs
Outdated
// SAFETY: In the signed case, the minimum integer is negative, and therefore non-zero. | ||
// SAFETY: In the unsignedd case, we use one, which is non-zero. | ||
pub const MIN : $Ty = unsafe { $Ty::new_unchecked($MinVal)}; | ||
} |
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.
This closing brace, and the analogous one in nonzero_constants_signed
should be unindented by one level = four spaces. The rest looks fine.
library/core/src/num/nonzero.rs
Outdated
#[unstable(feature = "nonzero_min_max", issue = "89065")] | ||
#[doc = concat!("The maximum value for a`", stringify!($Ty), "` is the same as `", stringify!($Int), "`")] | ||
#[doc = concat!("assert_eq!(", stringify!($Ty), "::MAX, ", stringify!($Int), "::MAX);")] | ||
/// Note, while most integer types are defined for every whole number between MIN and |
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.
I think this should be either Note that, while
or Note: while
rather than Note, while
, but I'm not sure whether others would agree with that grammar nitpick.
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.
Yeah, I'd vote for Note: While
with a capital W
.
Co-authored-by: LingMan <LingMan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
} | ||
|
||
nonzero_constants_unsigned! { | ||
NonZeroU8(u8); |
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.
Indentation mismatch here.
It looks like this is failing because it needs to reference these types with a path. |
Ping from triage: |
Ping from triage: @rustbot label: +S-inactive |
Implement `MIN`/`MAX` constants for non-zero integers This adds the associated [`MIN`](https://doc.rust-lang.org/stable/std/primitive.usize.html#associatedconstant.MIN)/[`MAX`](https://doc.rust-lang.org/stable/std/primitive.usize.html#associatedconstant.MAX) constants to `NonZero{U,I}{8,16,32,64,128,size}`, requested in rust-lang#89065. This reimplements rust-lang#89077 due that PR being stagnant for 4 months. I am fine with closing this in favor of that one if the author revisits it. If so, I'd like to see that PR have the docs link to the `$Int`'s constants.
This implements the feature requested in #89065 . I wrote some tests, but it appears that non-stable features aren't supposed to be tested in the normal nonzero.rs test file. Is there a place to add tests for non-stable features?