diff --git a/src/fixed.h b/src/fixed.h index a602664974a..fbede318664 100644 --- a/src/fixed.h +++ b/src/fixed.h @@ -6,6 +6,7 @@ #include #include +#include template class fixedf { @@ -149,34 +150,31 @@ class fixedf { } friend fixedf operator/(const fixedf &a, const fixedf &b) { + Uint64 abs_a = std::abs(a.v), abs_b = std::abs(b.v); + bool is_neg = int(a.v < 0) ^ int(b.v < 0); // 128-bit divided by 64-bit, to make sure high bits are not lost - Sint64 quotient_hi = a.v >> (64 - FRAC); - Uint64 quotient_lo = a.v << FRAC; - Sint64 d = b.v; - int isneg = 0; - Sint64 remainder = 0; - - if (d < 0) { - d = -d; - isneg = 1; - } - + Uint64 quotient_hi = abs_a >> (64 - FRAC); + Uint64 quotient_lo = abs_a << FRAC; + Uint64 remainder = 0; for (int i = 0; i < 128; i++) { Uint64 sbit = (Uint64(1) << 63) & quotient_hi; + // The most significant bit of remainder is 0 because abs_b <= (Uint64(1) << 63) remainder <<= 1; - if (sbit) remainder |= 1; + if (sbit) { + remainder |= 1; + } // shift quotient left 1 { quotient_hi <<= 1; if (quotient_lo & (Uint64(1) << 63)) quotient_hi |= 1; quotient_lo <<= 1; } - if (remainder >= d) { - remainder -= d; + if (remainder >= abs_b) { + remainder -= abs_b; quotient_lo |= 1; } } - return (isneg ? -Sint64(quotient_lo) : quotient_lo); + return is_neg ? -quotient_lo : quotient_lo; } friend bool operator==(const fixedf &a, const fixedf &b) { return a.v == b.v; } friend bool operator!=(const fixedf &a, const fixedf &b) { return a.v != b.v; } diff --git a/src/test/TestFixed.cpp b/src/test/TestFixed.cpp new file mode 100644 index 00000000000..54997a7c694 --- /dev/null +++ b/src/test/TestFixed.cpp @@ -0,0 +1,41 @@ +// Copyright © 2008-2024 Pioneer Developers. See AUTHORS.txt for details +// Licensed under the terms of the GPL v3. See licenses/GPL-3.txt + +#include +#include "doctest.h" +#include "fixed.h" + +TEST_CASE("Fixed") +{ + SUBCASE("operator ==") + { + // 42.0 == 42.0 + CHECK(fixed(42, 1) == fixed(42, 1)); + // 123.0 != 321.0 + CHECK(fixed(123, 1) != fixed(321, 1)); + // 7.0 == 7.0 + CHECK(fixed(0x7'00000000) == fixed(7, 1)); + } + + SUBCASE("operator /") + { + // 3.0 / 3.0 == 1.0 + CHECK(fixed(3, 1) / fixed(3, 1) == fixed(1, 1)); + // Negative numerators used to cause incorrect results + // Example: -3.0 / 3.0 should be -1.0 + CHECK(fixed(-0x3'00000000) / fixed(3, 1) == fixed(-0x1'00000000)); + // Denominators greater then 0x40000000.00000000 or less then -0x40000000.00000000 used to cause incorrect results + // Example: 0x40000000.00000000 / 0x40000000.00000001 should return 0x00000000.ffffffff (because we round down) + CHECK(fixed(0x40000000'00000000) / fixed(0x40000000'00000001) == fixed(0x00000000'ffffffff)); + // There used to be an edge case that triggered a signed int overflow when negating + // the final output if the truncation to 64 bits and conversio to Sint64 produced INT64_MIN + // because -INT64_MIN is undefined behavour + // Example: 0x00000000.80000000 / -0x00000000.00000001 + CHECK(fixed(0x80000000) / fixed(-1) == fixed(INT64_MIN)); + // For completeness: + // 3.0 / -3.0 == -1.0 + CHECK(fixed(3, 1) / fixed(-0x3'00000000) == fixed(-0x1'00000000)); + // -3.0 / -3.0 == 1.0 + CHECK(fixed(-0x3'00000000) / fixed(-0x3'00000000) == fixed(1, 1)); + } +}