Fix bugs in fixed division

src/fixed.h

@@ -6,6 +6,7 @@
 
 #include <SDL_stdinc.h>
 #include <cassert>
+#include <cstdlib>
 
 template <int FRAC_BITS>
 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; }

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 <cstdint>
+#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));
+	}
+}