AcademySoftwareFoundation / AcademySoftwareFoundation/Imath
Imath::Vec should use "= default" for copy constructor and assignment operator.
- Dominant language
- C++
- Stars
- 488
- Forks
- 161
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 3
Description
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)`.
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
IMATH_HOSTDEVICE constexpr inline Vec3::Vec3 (const Vec3& v) IMATH_NOEXCEPT
: x (v.x),
y (v.y),
z (v.z)
{}
template
IMATH_HOSTDEVICE IMATH_CONSTEXPR14 inline const Vec3&
Vec3::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& Vec3::operator=(const Vec3&) [with T = float]’ does not match the expected signature
note: expected signature: ‘constexpr Vec3& Vec3::operator=(const Vec3&)’
```
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.
Contributor guide
Research direction
Locate the Vec2, Vec3, and Vec4 declarations and their copy-constructor and assignment-operator definitions referenced in the issue. Check the existing signatures and the trivially-copyable behavior, then verify the relevant C++ tests or a static_assert for the vector types. Done means the operators use the requested defaulted forms and the assignment operator returns a non-const reference.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- computer-graphics
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 42/100