MikeLankamp / fpm

C++ header-only fixed-point math library
https://mikelankamp.github.io/fpm
MIT License
672 stars 85 forks source link

non-constexpr function 'round' #26

Closed Klummel69 closed 3 years ago

Klummel69 commented 3 years ago

The following code generates an error message under different clang versions, because std::round() is not a constexpr function in C++11/14.

static constexpr fpm::fixed_16_16 delta {1.234};
//error: constexpr variable 'delta' must be initialized by a constant expression
//static constexpr fpm::fixed_16_16 delta {1.234};
//                                  ^~~~~~~~~~~~~
//.\include\fpm/fixed.hpp: note: non-constexpr function 'round' cannot be used in a constant expression
//        : m_value(static_cast<BaseType>(std::round(val * FRACTION_MULT)))

I suggest the following workaround: replace std::round()

    // Converts an floating-point number to the fixed-point type.
    // Like static_cast, this truncates bits that don't fit.
    template <typename T, typename std::enable_if<std::is_floating_point<T>::value>::type* = nullptr>
    constexpr inline explicit fixed(T val) noexcept
        //: m_value(static_cast<BaseType>(std::round(val * FRACTION_MULT)))
        : m_value(static_cast<BaseType>((val >= 0.0) ? (val * FRACTION_MULT + 0.5) : (val * FRACTION_MULT - 0.5)))
    {}

On Godbolt you can see the behavior (without fpm header)

https://godbolt.org/z/34cns5fnK

Advantage: This allows constants to be defined at compile time without the detour via from_fixed_point<..>....

MikeLankamp commented 3 years ago

Thanks for reporting this! Your suggested fix looks good as well.

This allows constants to be defined at compile time without the detour via from_fixed_point<..>....

True, but using this constructor would still rely on floats during compilation. I'm not confident enough that every compiler's compile-time handling of floats is the same. Removing floating point numbers in every step of the process is safer.

Klummel69 commented 3 years ago

True, but using this constructor would still rely on floats during compilation. I'm not confident enough that every compiler's compile-time handling of floats is the same. Removing floating point numbers in every step of the process is safer.

I don't think this is a problem. As far as I know constexpr variables must be resolved at compile time. Therefore no float operations should be left at runtime. I use a compiler here that allows float at compile time but throws errors when using float at runtime.

Alternatively you could define a method "fraction", which use 2 numbers in fraction notation (numerator by denominator). This would give us a good representation of a floating/fixed point number.

// Example
static constexpr fpm::fixed_16_16 delta = fpm::fixed_16_16.fraction(1234, 1000);     // Or similar...

Thanks for the update, as always blazing fast!