Replace =delete with SFINAE for invalid Vec integer methods

[chinmaychahar]

Summary

Fixes #559

Replace = delete template specializations with SFINAE using the existing IMATH_ENABLE_IF macro for length(), normalize(), normalizeExc(), normalizeNonNull(), normalized(), normalizedExc(), and normalizedNonNull() in Vec2, Vec3, and Vec4.

Removes the entire Specializations for VecN<short>, VecN<int> block including the @cond Doxygen_Suppress wrapper which will no longer be needed when SFINAE makes the methods invisible to both the compiler and Doxygen for non-applicable types.

Status

Draft PR. I will iterate based on feedback and open questions in #559

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: chinmaychahar / name: chinmaychahar (33e84ff)

[lgritz]

I think it's probably unwise to rely on either std::is_floating_point or std::is_integral. We should probably define our own extensible type trait that will capture what we really intend and be user-extensible and future-proof.

[chinmaychahar]

Thanks @lgritz, I added an is_float_like that defaults to std::is_floating_point with a specialization for half. Let me know if this is along the lines of what you had in mind

[chinmaychahar]

Could a reviewer please take a look at this? Wouldn't want it go stale

[cary-ilm]

Apologies for the delay, I've been consumed with other issues. And thanks for you continued efforts! I'm happy to work through the issues.

The implementation needs to support Imath::Vec3<Foo> declarations for any externally-declared type Foo, not just half. The library needs to provide a way for the user to give a specialization for their own class. It may be rare, but it shouldn't be made impossible.

I think something like this is the right direction:

In ImathTypeTraits.h:

#if (IMATH_CPLUSPLUS_VERSION >= 17)
using std::void_t;
#else
template <typename...> using void_t = void;
#endif

/// True if ``T`` supports the arithmetic operations used by ``Vec::length()``.
template <typename T, typename = void> struct imath_supports_vec_length : std::false_type
{};

template <typename T>
struct imath_supports_vec_length<
    T,
    void_t<
        decltype (std::declval<T> () * std::declval<T> ()),
        decltype (std::declval<T> () + std::declval<T> ()),
        decltype (std::declval<T> () / std::declval<T> ()),
        decltype (std::declval<T> () < std::declval<T> ()),
        decltype (T (0)),
        decltype (T (2)),
        decltype (std::numeric_limits<T>::min ()),
        decltype (std::sqrt (std::declval<T> ())),
        decltype (std::abs (std::declval<T> ()))>> : std::true_type
{};

/// True for scalar types suitable for ``Vec`` length/normalize.
template <typename T>
struct imath_is_floating_point
    : std::integral_constant<
          bool,
          imath_supports_vec_length<T>::value && !std::is_integral<T>::value>
{};

and then in ImathVec.h:

#define IMATH_IF_FLOATING_POINT                                                  \
    template <                                                                 \
        typename U = T,                                                          \
        typename std::enable_if<                                                 \
            imath_is_floating_point<U>::value,                                  \
            int>::type = 0>

#define IMATH_IF_FLOATING_POINT_IMPL                                          \
    template <                                                                \
        typename U,                                                           \
        typename std::enable_if<                                                \
            imath_is_floating_point<U>::value,                                \
            int>::type>

and then later:

    IMATH_IF_FLOATING_POINT
    IMATH_HOSTDEVICE T length () const IMATH_NOEXCEPT;

and finally:

template <class T>
IMATH_IF_FLOATING_POINT_IMPL
IMATH_HOSTDEVICE inline T
Vec2<T>::length () const IMATH_NOEXCEPT
{
    T length2 = dot (*this);

    if (IMATH_UNLIKELY (length2 < T (2) * std::numeric_limits<T>::min ()))
        return lengthTiny ();

    return std::sqrt (length2);
}

Also note that this needs to work with C++17 through C++26, and unfortunately some of these trait template change signature, so it's a little tricky.

It would also be good to add CI jobs that build with the various C++ standards.

Also, be sure to configure your fork of the repo to run the CI on your local branch. That way, you can confirm it works before pushing to the PR. When I find myself in a situation like this where I have to make substantial changes to an already-submitted PR, I often create a second branch, work there until I'm happy, then squash the commits and cherry-pick them over to the branch for the PR. That avoids a lot of noise on the PR.

[lgritz]

template <typename T>
struct imath_supports_vec_length<
    T,
    void_t<
        decltype (std::declval<T> () * std::declval<T> ()),
        decltype (std::declval<T> () + std::declval<T> ()),
        decltype (std::declval<T> () / std::declval<T> ()),
        decltype (std::declval<T> () < std::declval<T> ()),
        decltype (T (0)),
        decltype (T (2)),
        decltype (std::numeric_limits<T>::min ()),
        decltype (std::sqrt (std::declval<T> ())),
        decltype (std::abs (std::declval<T> ()))>> : std::true_type
{};

What the heck does that mean?