Wrapping shift operator code doing bitmasking twice

internals
1

This question starts from this clippy output:

warning: suspicious use of `&` in `Shl` impl
   --> library/core/src/num/wrapping.rs:152:53
    |
144 | / macro_rules! sh_impl_unsigned {
145 | |     ($t:ident, $f:ident) => {
146 | |         #[stable(feature = "rust1", since = "1.0.0")]
147 | |         impl Shl<$f> for Wrapping<$t> {
...   |
152 | |                 Wrapping(self.0.wrapping_shl((other & self::shift_max::$t as $f) as u32))
    | |                                                     ^

coming from this code:

macro_rules! sh_impl_unsigned {
    ($t:ident, $f:ident) => {
        #[stable(feature = "rust1", since = "1.0.0")]
        impl Shl<$f> for Wrapping<$t> {
            type Output = Wrapping<$t>;

            #[inline]
            fn shl(self, other: $f) -> Wrapping<$t> {
                Wrapping(self.0.wrapping_shl((other & self::shift_max::$t as $f) as u32))
            }
        }

which calls into:

        #[inline(always)]
        pub const fn wrapping_shl(self, rhs: u32) -> Self {
            // SAFETY: the masking by the bitsize of the type ensures that we do not shift
            // out of bounds
            unsafe {
                self.unchecked_shl(rhs & (Self::BITS - 1))
            }
        }

So I'm wondering if there is any purpose that I'm missing to doing (other & self::shift_max::$t as $f) since this bitmasking seems to be repeated in wrapping_shl. Shift_max is coming from this code:

mod shift_max {
    #![allow(non_upper_case_globals)]

    #[cfg(target_pointer_width = "16")]
    mod platform {
        pub(crate) const usize: u32 = super::u16;
        pub(crate) const isize: u32 = super::i16;
    }

    #[cfg(target_pointer_width = "32")]
    mod platform {
        pub(crate) const usize: u32 = super::u32;
        pub(crate) const isize: u32 = super::i32;
    }

    #[cfg(target_pointer_width = "64")]
    mod platform {
        pub(crate) const usize: u32 = super::u64;
        pub(crate) const isize: u32 = super::i64;
    }

    pub(super) const i8: u32 = (1 << 3) - 1;
    pub(super) const i16: u32 = (1 << 4) - 1;
    pub(super) const i32: u32 = (1 << 5) - 1;
    pub(super) const i64: u32 = (1 << 6) - 1;
    pub(super) const i128: u32 = (1 << 7) - 1;
    pub(super) use self::platform::isize;

    pub(super) const u8: u32 = i8;
    pub(super) const u16: u32 = i16;
    pub(super) const u32: u32 = i32;
    pub(super) const u64: u32 = i64;
    pub(super) const u128: u32 = i128;
    pub(super) use self::platform::usize;
}
2

Good catch! This looks like an oversight to me. Before Fix a breaking change in #30523 by strega-nil · Pull Request #30733 · rust-lang/rust · GitHub, the masking was needed to avoid a panic, and when that PR ported the code to use wrapping_shl instead of << they just kept the mask. I don't even know why that PR introduced wrapping_shl, it seems entirely orthogonal to the stated goal of the PR.

The Add etc impls already just use self.0.wrapping_add without further processing, I think the shift impls should do the same.

1 Like