BigUglySpider / EmuLibs

Selection of libraries designed to be used with Emu projects. This was originally a Math library only, but has since been changed to hold all Emu libraries to enable consistency in changes to dependencies (such as EmuCore modifications).
https://biguglyspider.github.io/math
0 stars 0 forks source link

Bad enable_if triggers on some constructors #50

Closed BigUglySpider closed 2 years ago

BigUglySpider commented 2 years ago

Overview

There are some uses of std::enable_if/std::enable_if_t that are ill-formed, and result in compiler errors.

Under MSVC (the test environment), errors are reported but compilation is still successful. However, this is just a ticking time bomb.

Problem Area

This use is primarily in constructors that use std::enable_if/std::enable_if_t with a constant condition, such as the following example in Vector:

template<std::size_t Size_, typename T_>
struct Vector
{
    ...

    template<typename = std::enable_if_t<has_alternative_rep>>
    constexpr Vector(const alternative_rep& to_copy_) ...
    {
        ...
    }
    ...
};

As this is a constant check, we are technically always mentioning the enable_if's ::type for any Vector type instantiation. In cases where has_alternative_rep == false (which is the common case), we're referencing an item that does not exist.

Resolution

An alternative approach to this is similar to the use of EmuCore::get_false for always-false static_assert calls: we use a dummy template argument that doesn't actually get used, but is involved in the condition in some way (even as just another dummy template argument for a deferred constexpr function call).

See the following use of std::enable_if_t in the most recent Quaternion Constructors, which follow this approach:

template<typename T_>
struct Quaternion
{
    ...

    template<std::size_t Unused_ = 0, typename = std::enable_if_t<is_default_constructible<Unused_>()>>
    constexpr inline Quaternion() : data()
    {
    }
    ...
};

Side Notes

Although this isn't directly a problem (or, at least, does not appear to be as of the time of writing), a shift in preference has been made for where std::enable_if_t is used for functions. Previously, it was as an additional unnamed typename in the template arguments.

For clarity's sake, what was previously written as

template<typename...SomeArgs_, typename = std::enable_if_t<some_condition<SomeArgs_...>()>>
return_type some_function(SomeArgs_&&...args_)
{
   ...
}

Should now preferably be written as

template<typename...SomeArgs_>
auto some_function(SomeArgs_&&...args_)
    -> std::enable_if_t<some_condition<SomeArgs_...>(), return_type>
{
   ...
}

This then moves the condition out of the template arguments to remove any confusion when providing explicit template arguments, as well as decoupling the enable_if condition from a separate component of the function.

BigUglySpider commented 2 years ago

About the side notes:

This preference does not apply to constructors and classes/structs, as we can only use it in the template arguments for the constructor. Although, for the use of classes and structs, one should prefer an internal static_assert over a enable_if trigger for most cases. In scenarios where it is being used for SFINAE specialisation, consider alternatives such as if constexpr first.

BigUglySpider commented 2 years ago

Corrections have been made to known culprits as of merge #56