vpiotr / decimal_for_cpp

Decimal data type for C++
273 stars 68 forks source link

incorrect constructing from fp #33

Closed mmkbbt closed 5 years ago

mmkbbt commented 5 years ago

incorrect constructing from fp

dec::decimal2(9999999999999998.0); // 9999999999999997.44
dec::decimal4(9999999999999998.0); // -92233720368547758.08

incorrect round dec::decimal2(3.1549999999999998); // must be 3.15, but 3.16

test condition and test condition is incorrect

vpiotr commented 5 years ago

Case 1: dec::decimal2(9999999999999998.0) is not representable in this data type. Maximum value is 2^63-1 divided by 10^precision, so something closer to 9,223372036854775808e16 in this case.

Case 2: See case 1.

Case 3: Using double constructor is not recommended and only useful for numbers with not too many significant digits.

In this case you are trying to use 17 digits with double which supports 15 to 17 significant decimal digits. In addition, construction from double requires additional digits in the middle of conversion (= precision). So in this case 17 + 2 = 19 digits are required from double. See sample code: https://ideone.com/GNTtuF Wiki describing double limits: https://en.wikipedia.org/wiki/Double-precision_floating-point_format

For such case a special constructor has been created which supports better so many digits (string constructor).

Code:


    dec::decimal<2> d1(3.1549999999999998);
    std::cout << "Value #1 is: " << d1 << std::endl;
    dec::decimal<2> d2("3.1549999999999998");
    std::cout << "Value #2 is: " << d2 << std::endl;

outputs:

    Value #1 is: 3.16
    Value #2 is: 3.15
mmkbbt commented 5 years ago

posible partial fix for line is

   void init(double value) {
        dec_storage_t t_v0 = static_cast<dec_storage_t>(value);
        m_value = RoundPolicy::round(
                static_cast<double>(DecimalFactor<Prec>::value) * (value - t_v0)) +
                DecimalFactor<Prec>::value * t_v0;
    }
vpiotr commented 5 years ago

Implemented fix as suggested, corrected unit tests, thanks!

mmkbbt commented 5 years ago

same bug in line. pls fix too

vpiotr commented 5 years ago

Assignment corrected too.