Implementing Mul and MulAssign on type that is Copy

I have a custom rational type that doesn't need to be generic:

#[derive(Copy, Clone, Debug, PartialEq)]
pub struct Chance {
    numerator: usize,
    denominator: usize,
}

I am implementing math operations on it, and at first I was doing what was obvious at first and implementing everything with references:

impl ops::Mul<Self> for &Chance {
    type Output = Chance;

    fn mul(self, rhs: Self) -> Self::Output { ... }
}

However, when implementing MulAssign I realized that since the type is Copy, I could just implement it without references, and mutate self, which is automatically copied when calling, so the same amount of allocation is happening regardless. It also means I could just call my implementation of MulAssign from Mul.

So my questions are:

  • Is this in general frowned upon?
  • Is my understanding of the interaction of taking mut self on a Copy type wrong in some way?
  • Is there something else that I'm blatantly missing?

The full ending code for reference:

#[derive(Copy, Clone, Debug, PartialEq)]
pub struct Chance {
    numerator: usize,
    denominator: usize,
}

impl ops::Mul<&Self> for Chance {
    type Output = Self;

    fn mul(mut self, rhs: &Self) -> Self::Output {
        self *= rhs;
        self
    }
}

impl ops::MulAssign<&Self> for Chance {
    fn mul_assign(&mut self, rhs: &Self) {
        self.numerator *= rhs.numerator;
        self.denominator *= rhs.denominator;
    }
}

"Two machine words/pointer-sized values" is just about the exact point at which the value is small enough that passing it by move is definitely more efficient.[1] Therefore, if you have only one set of operator implementations, it should be the one that takes values and not references. But providing both versions (as the standard library number types do) provides convenience.


  1. If it were any larger, then the compiler would be passing it by pointer implicitly regardless. ↩ī¸Ž

3 Likes

That makes sense as far as the optimization of the implementation goes, but I'm mainly focused on whether implementing the non-mutative operations via implicit copy and mutation has any hidden consequences I'm not thinking of. Also just the general question if this is normal for an implementation detail or in general people would find this code "surprising" and avoid it for more straightforward immutable creation of a return object.

Yeah, mutating an argument that you take by value (or not) is just an implementation detail. It doesn't effect the API or anything.

1 Like

No. It's fine.

You can also take the rhs argument by value for symmetry:

impl ops::Mul<Self> for Chance

You can also go the other way: implement MulAssign in terms of Mul. For multiplication either approach is equally clear, but for addition it seems more natural that way:

impl Add<Self> for Chance {
    type Output = Self;

    fn add(self, rhs: Self) -> Self {
        Self {
            numerator: self.numerator * rhs.denominator + self.denominator * rhs.numerator,
            denominator: self.denominator * rhs.denominator,
        }
    }
}

impl AddAssign<Self> for Chance {
    fn add_assign(&mut self, rhs: Self) {
        *self = *self + rhs;
    }
}

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.