Imath icon indicating copy to clipboard operation
Imath copied to clipboard

Imath::Vec should use "= default" for copy constructor and assignment operator.

Open abucior-ilm opened this issue 1 year ago • 1 comments

I'd like to propose that we = default the copy constructor and assignment operator for the Vec types. My motivation is that I've been working on some code recently where I'm defining a template that should only work with types that can be memcpy()'ed. It seems the recommended approach to ensure a type is memcpy'able is to static_assert(std::is_trivially_copyable_v<T>). Surprisingly, I'm finding that Imath::V3f (and any of the other similar types) fails this test. Just looking at the class, it's obvious that a byte-for-byte copy would be totally fine. It fails the test because the strict definition of a trivially copyable type includes requirements that the copy and assignment operators are also "trivial", which means they cannot be user defined. In the case of Imath::Vec2/3/4, they are user-defined, but perhaps needlessly so. I'm fairly sure they could be " = default":

template <class T>
IMATH_HOSTDEVICE constexpr inline Vec3<T>::Vec3 (const Vec3& v) IMATH_NOEXCEPT
    : x (v.x),
      y (v.y),
      z (v.z)
{}

template <class T>
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 inline const Vec3<T>&
Vec3<T>::operator= (const Vec3& v) IMATH_NOEXCEPT
{
    x = v.x;
    y = v.y;
    z = v.z;
    return *this;
}

Also, related to this, I believe this declaration might be incorrect:

    IMATH_HOSTDEVICE IMATH_CONSTEXPR14 const Vec3&
    operator= (const Vec3& v) IMATH_NOEXCEPT;

Specifically, that initial const should probably not be there. With the const in place, you can't " = default" and it results in errors that look like the following:

defaulted declaration ‘constexpr const Vec3<T>& Vec3<T>::operator=(const Vec3<T>&) [with T = float]’ does not match the expected signature
note: expected signature: ‘constexpr Vec3<float>& Vec3<float>::operator=(const Vec3<float>&)’

Note the missing "const" in the expected signature.

The canonical implementation of operator= does not normally return a const reference and simply returns a reference.

My suggestion would be to change the Vec declarations to use " = default" and remove the extra "const". As an example:

    IMATH_HOSTDEVICE constexpr Vec3 (const Vec3& v) IMATH_NOEXCEPT = default;

    IMATH_HOSTDEVICE IMATH_CONSTEXPR14 Vec3&
    operator= (const Vec3& v) IMATH_NOEXCEPT = default;

... and remove the definitions for those functions.

abucior-ilm avatar Dec 18 '24 20:12 abucior-ilm

That suggestion makes sense to me.

meshula avatar Dec 21 '24 02:12 meshula