implement ZeroableOption for NonZero* types#110
implement ZeroableOption for NonZero* types#110Hamdan-Khan wants to merge 1 commit intoRust-for-Linux:mainfrom
Conversation
BennoLossin
left a comment
BennoLossin
left a comment
There was a problem hiding this comment.
Thanks a lot for the PR!
Please use the kernel's commit message style. So give a motivation in addition to explaining what the change is.
We also don't use the issue: tag. Instead use Link:.
Please also mention this change in the changelog.
| ($($int:ty),* $(,)?) => { | ||
| // SAFETY: All zeros is equivalent to `None` (option layout optimization guarantee: | ||
| // <https://doc.rust-lang.org/stable/std/option/index.html#representation>). | ||
| $(unsafe impl ZeroableOption for $int {})* |
There was a problem hiding this comment.
This safety comment should go next to the NonZero* types instead of being here. You should still keep a safety comment here though, similar to the impl_zeroable! definition.
src/lib.rs
Outdated
| impl_non_zero_int_zeroable_option!( | ||
| NonZeroU8, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU128, NonZeroUsize, | ||
| NonZeroI8, NonZeroI16, NonZeroI32, NonZeroI64, NonZeroI128, NonZeroIsize, | ||
| ); |
There was a problem hiding this comment.
Also use {} instead of (), since otherwise rustfmt will try to format this to be one line per item.
BennoLossin
left a comment
BennoLossin
left a comment
There was a problem hiding this comment.
The code itself looks good. One suggestion for the changelog inline.
Lastly, we should offer some motivation for the change in the commit message. The idea is that Option<NonZero*> automatically implements Zeroable through the blanket impl that comes from implementing ZeroableOption for NonZero*, better encoding the situation.
CHANGELOG.md
Outdated
| - `[pin_]init_scope` functions to run arbitrary code inside of an initializer. | ||
| - `&'static mut MaybeUninit<T>` now implements `InPlaceWrite`. This enables users to use external | ||
| allocation mechanisms such as `static_cell`. | ||
| - Add a macro to implement `ZeroableOption` for non-zero integer types (i.e. `NonZero*`). |
There was a problem hiding this comment.
The changelog should only contain user-visible changes. The macro is private and implementation detail, so that shouldn't be mentioned here. Instead the relevant part is that NonZero* now implements ZeroableOption.
add a macro for implementing `ZeroableOption` for `NonZero*` types. `Option<NonZero*>` now automatically implements `Zeroable` trait by implementing `ZeroableOption` for `NonZero*` types, which serves as a blanket impl. Link: Rust-for-Linux#95 Signed-off-by: Hamdan-Khan <hamdankhan212@gmail.com>
2 participants
add a macro for implementing ZeroableOption for NonZero* integer types instead of using Option<NonZero*> inside zeroable macro
issue: #95