MikeLankamp / fpm

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

fix issue #33:Conversion of max() and lowest() to float #35

Closed TxtpGame closed 2 years ago

MikeLankamp commented 2 years ago

Hi @TxtpGame, sorry for the delay. I had to think about your proposed change. This change effectively forces at least 2 integral bits, giving a range of -2.0 to 1.999.... So I wonder: why not reduce this to at least 1 bit (since we need at least the sign bit), giving a range of -1.0 to 0.999...? That feels like a more logical limit.

I'd also appreciate a test or two to make sure a Q1.15 number works correctly, being an edge case.

TxtpGame commented 2 years ago
  1. we need at least the sign bit. so this limit is changed to sizeof(BaseType) * 8 - 1

  2. FRACTION_MULT participated in the intermediate operation. so it's type change to IntermediateType, is more reasonable. eg

    using fixed_0_15 = fpm::fixed<std::int16_t, std::int32_t, 15>;
    auto max = std::numeric_limits<fixed_0_15>::max();
    auto lowest = std::numeric_limits<fixed_0_15>::lowest();
    std::cout << "fixed_0_15"<< " max " << max<< " lowest " << lowest<< std::endl;
    std::cout << "int(fixed_0_15)"<< " max " << int(max)<< " lowest " << int(lowest)<< std::endl;
    std::cout << "float(fixed_0_15)"<< " max " << float(max)<< " lowest " << float(lowest)<< std::endl;

    when FRACTION_MULT is BaseType, this output is error

    fixed_0_15 max 0.999969 lowest -1
    int(fixed_0_15) max 0 lowest 1
    float(fixed_0_15) max -0.999969 lowest 1

when FRACTION_MULT is IntermediateType, this output is OK

fixed_0_15 max 0.999969 lowest -1
int(fixed_0_15) max 0 lowest -1
float(fixed_0_15) max 0.999969 lowest -1
MikeLankamp commented 2 years ago

Thanks for identifying the fix, @TxtpGame! This PR has been replaced by PR #40, which is basically the same fix, but includes tests.