Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
abucior-ilm opened this issue Dec 18, 2024 · 1 comment

Comments

@abucior-ilm
Copy link

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 abucior-ilm changed the title Imath::Vec should use " = default" for copy constructor and assignment operator. Imath::Vec should use "= default" for copy constructor and assignment operator. Dec 18, 2024
@meshula
Copy link
Contributor

meshula commented Dec 21, 2024

That suggestion makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants