servo / euclid

Geometry primitives (basic linear algebra) for Rust
Other
462 stars 102 forks source link

Why cloned scale is used only for the first coordinate? #439

Closed stephanemagnenat closed 4 years ago

stephanemagnenat commented 4 years ago

In several part of euclid's source code, for example in Point2D multiplied by a scale, the scale is cloned and applied to x, but applied to y uncloned:

impl<T: Clone + Mul, U> Mul<T> for Point2D<T, U> {
    type Output = Point2D<T::Output, U>;

    #[inline]
    fn mul(self, scale: T) -> Self::Output {
        point2(self.x * scale.clone(), self.y * scale)
    }
}

I do not understand this difference. Could it be a bug, copy-pasted in multiple places? Would the right implementation be:

    fn mul(self, scale: T) -> Self::Output {
        let cloned_scale = scale.clone();
        point2(self.x * cloned_scale, self.y * cloned_scale)
    }

? Being it so pervasive, I doubt it is a bug, yet I cannot see a reason, hence this issue.

emilio commented 4 years ago

What you propose would not build, for the same reason this doesn't build point2(self.x * scale, self.y * scale). You need to clone the scale once so you can use it again. But you don't need to clone it twice, thus the current thing.

In practice for types that are both Clone and Copy, there's no difference.