From 8d0afac1943a5bffc4abe463db99fe78978ddb99 Mon Sep 17 00:00:00 2001 From: ephphatha Date: Tue, 14 Jun 2022 19:40:23 +1000 Subject: [PATCH] Don't preserve an unreachable vanilla bug --- Source/engine/random.cpp | 2 +- test/random_test.cpp | 31 +++++++++++++++++-------------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/Source/engine/random.cpp b/Source/engine/random.cpp index 3b5d4d6cb..fa11f74a5 100644 --- a/Source/engine/random.cpp +++ b/Source/engine/random.cpp @@ -46,7 +46,7 @@ int32_t GenerateRnd(int32_t v) { if (v <= 0) return 0; - if (v < 0xFFFF) + if (v <= 0x7FFF) // use the high bits to correct for LCG bias return (AdvanceRndSeed() >> 16) % v; return AdvanceRndSeed() % v; } diff --git a/test/random_test.cpp b/test/random_test.cpp index a2578bff8..dc2f6ea26 100644 --- a/test/random_test.cpp +++ b/test/random_test.cpp @@ -113,20 +113,19 @@ TEST(RandomTest, ShiftModDistributionSingleRange) ASSERT_EQ(GenerateRnd(1), 0) << "Interval [0, 1) must return 0 when AbsDistribution yields INT_MIN"; } -// When called with an upper bound less than 0xFFFF this distribution function discards the low 16 bits of the output +// When called with an upper bound less than or equal to 0x7FFF this distribution function discards the low 16 bits of the output // from the default distribution by performing a shift right of 16 bits. This relies on implementation defined // behaviour for negative numbers, the expectation is shift right uses sign carry. See C++17 [expr.shift] TEST(RandomTest, ShiftModDistributionSignCarry) { - // This distribution is used when the upper bound is a value in [1, 65535) + // This distribution is used when the upper bound is a value in [1, 32768) // Using an upper bound of 1 means the result always maps to 0, see RandomTest_ShiftModDistributionSingleRange - // The only negative value return from AbsDistribution is -2147483648 + // The only negative value returned from AbsDistribution is -2147483648 // A sign-preserving shift right of 16 bits gives a result of -32768. - SetRndSeed(1457187811); // Test mod with no division - ASSERT_EQ(GenerateRnd(65535 - 1), -32768) << "Distribution must map negative numbers using sign carry shifts"; + // This is greater in magnitude than the limit so always results in a division. SetRndSeed(1457187811); // Test mod when a division occurs - ASSERT_EQ(GenerateRnd(32768 - 1), -1) << "Distribution must map negative numbers using sign carry shifts"; + ASSERT_EQ(GenerateRnd(32767), -1) << "Distribution must map negative numbers using sign carry shifts"; } // The Diablo LCG implementation attempts to improve the quality of generated numbers that would only use the low @@ -134,9 +133,9 @@ TEST(RandomTest, ShiftModDistributionSignCarry) // be an inconsistency with the implementation in devilutionx, see the comment for RandomTest_ShiftModDistributionHighBits TEST(RandomTest, ShiftModDistributionLowBits) { - // All the following seeds generate values less than 2^16, so after shifting they give a 0 value - constexpr auto maxBound = 65534; + constexpr auto maxBound = 32767; + // All the following seeds generate values less than 2^16, so after shifting they give a 0 value SetRndSeed(3604671459U); // yields 0 ASSERT_EQ(GenerateRnd(maxBound), 0) << "Invalid distribution when generator yields 0"; SetRndSeed(0); // yields 1 @@ -187,7 +186,7 @@ TEST(RandomTest, ShiftModDistributionLowBits) // i.e., no cast from unsigned to signed, no modulo when building the return value. TEST(RandomTest, ShiftModDistributionHighBits) { - constexpr auto maxBound = 65534; + constexpr auto maxBound = 32767; SetRndSeed(3267226595U); // yields 65536 ASSERT_EQ(GenerateRnd(maxBound), 1) << "Invalid distribution when generator yields 65536"; SetRndSeed(3942116323U); // yields -65536 @@ -213,21 +212,25 @@ TEST(RandomTest, ShiftModDistributionHighBits) SetRndSeed(2724598777); // yields -1212022642 ASSERT_EQ(GenerateRnd(maxBound), 18493) << "Invalid distribution when generator yields -1212022642"; SetRndSeed(76596137); // yields 2147483646 - ASSERT_EQ(GenerateRnd(maxBound), 32767) << "Invalid distribution when generator yields 2147483646"; + ASSERT_EQ(GenerateRnd(maxBound), 0) << "Invalid distribution when generator yields 2147483646"; SetRndSeed(2837779485); // yields -2147483646 - ASSERT_EQ(GenerateRnd(maxBound), 32767) << "Invalid distribution when generator yields -2147483646"; + ASSERT_EQ(GenerateRnd(maxBound), 0) << "Invalid distribution when generator yields -2147483646"; SetRndSeed(766891974); // yields 2147483647 - ASSERT_EQ(GenerateRnd(maxBound), 32767) << "Invalid distribution when generator yields 2147483647"; + ASSERT_EQ(GenerateRnd(maxBound), 0) << "Invalid distribution when generator yields 2147483647"; SetRndSeed(2147483648); // yields -2147483647 - ASSERT_EQ(GenerateRnd(maxBound), 32767) << "Invalid distribution when generator yields -2147483647"; + ASSERT_EQ(GenerateRnd(maxBound), 0) << "Invalid distribution when generator yields -2147483647"; } TEST(RandomTest, ModDistributionSignPreserving) { - // This distribution is used when the upper bound is a value in [65535, 2147483647] + // This distribution is used when the upper bound is a value in [32768, 2147483647] // Sign preserving modulo when no division is performed cannot be tested in isolation with the current implementation. SetRndSeed(1457187811); + ASSERT_EQ(GenerateRnd(32768), 0) << "Distribution must map negative numbers using sign preserving modulo"; + SetRndSeed(1457187811); + ASSERT_EQ(GenerateRnd(32769), -2) << "Distribution must map negative numbers using sign preserving modulo"; + SetRndSeed(1457187811); ASSERT_EQ(GenerateRnd(65535), -32768) << "Distribution must map negative numbers using sign preserving modulo"; SetRndSeed(1457187811); ASSERT_EQ(GenerateRnd(std::numeric_limits::max()), -1)