Use safer versions of random number functions instead of GenerateRnd

Source/effects.cpp

@@ -1026,8 +1026,6 @@ void PlaySfxPriv(TSFX *pSFX, bool loc, Point position)
 
 _sfx_id RndSFX(_sfx_id psfx)
 {
-	int nRand;
-
 	switch (psfx) {
 	case PS_WARR69:
 	case PS_MAGE69:
@@ -1037,8 +1035,7 @@ _sfx_id RndSFX(_sfx_id psfx)
 	case LS_ACID:
 	case IS_MAGIC:
 	case IS_BHIT:
-		nRand = 2;
-		break;
+		return PickRandomlyAmong({ psfx, static_cast<_sfx_id>(psfx + 1) });
 	case PS_WARR14:
 	case PS_WARR15:
 	case PS_WARR16:
@@ -1046,13 +1043,10 @@ _sfx_id RndSFX(_sfx_id psfx)
 	case PS_ROGUE14:
 	case PS_MAGE14:
 	case PS_MONK14:
-		nRand = 3;
-		break;
+		return PickRandomlyAmong({ psfx, static_cast<_sfx_id>(psfx + 1), static_cast<_sfx_id>(psfx + 2) });
 	default:
 		return psfx;
 	}
-
-	return static_cast<_sfx_id>(psfx + GenerateRnd(nRand));
 }
 
 void PrivSoundInit(uint8_t bLoadMask)

Source/engine/palette.cpp

@@ -230,12 +230,12 @@ void LoadRndLvlPal(dungeon_type l)
 		return;
 	}
 
-	int rv = GenerateRnd(4) + 1;
 	if (l == DTYPE_CRYPT) {
 		LoadPalette("nlevels\\l5data\\l5base.pal");
 		return;
 	}
 
+	int rv = RandomIntBetween(1, 4);
 	char szFileName[27];
 	if (l == DTYPE_NEST) {
 		if (!*sgOptions.Graphics.alternateNestArt) {

Source/items.cpp

@@ -4215,13 +4215,13 @@ void SpawnSmith(int lvl)
 	constexpr int PinnedItemCount = 0;
 
 	int maxValue = 140000;
-	int maxItems = 20;
+	int maxItems = 19;
 	if (gbIsHellfire) {
 		maxValue = 200000;
-		maxItems = 25;
+		maxItems = 24;
 	}
 
-	int iCnt = GenerateRnd(maxItems - 10) + 10;
+	int iCnt = RandomIntBetween(10, maxItems);
 	for (int i = 0; i < iCnt; i++) {
 		Item &newItem = smithitem[i];
 
@@ -4283,9 +4283,8 @@ void SpawnWitch(int lvl)
 	constexpr std::array<_item_indexes, MaxPinnedBookCount> PinnedBookTypes = { IDI_BOOK1, IDI_BOOK2, IDI_BOOK3, IDI_BOOK4 };
 
 	int bookCount = 0;
-	const int pinnedBookCount = gbIsHellfire ? GenerateRnd(MaxPinnedBookCount) : 0;
-	const int reservedItems = gbIsHellfire ? 10 : 17;
-	const int itemCount = GenerateRnd(WITCH_ITEMS - reservedItems) + 10;
+	const int pinnedBookCount = gbIsHellfire ? RandomIntLessThan(MaxPinnedBookCount) : 0;
+	const int itemCount = RandomIntBetween(10, gbIsHellfire ? 24 : 17);
 	const int maxValue = gbIsHellfire ? 200000 : 140000;
 
 	for (int i = 0; i < WITCH_ITEMS; i++) {
@@ -4461,9 +4460,9 @@ void SpawnHealer(int lvl)
 {
 	constexpr int PinnedItemCount = 2;
 	constexpr std::array<_item_indexes, PinnedItemCount + 1> PinnedItemTypes = { IDI_HEAL, IDI_FULLHEAL, IDI_RESURRECT };
-	const int itemCount = GenerateRnd(gbIsHellfire ? 10 : 8) + 10;
+	const int itemCount = RandomIntBetween(10, gbIsHellfire ? 19 : 17);
 
-	for (int i = 0; i < 20; i++) {
+	for (int i = 0; i < sizeof(healitem) / sizeof(healitem[0]); i++) {
 		Item &item = healitem[i];
 		item = {};
 
@@ -4849,15 +4848,14 @@ void RechargeItem(Item &item, Player &player)
 	if (item._iCharges == item._iMaxCharges)
 		return;
 
-	int r = GetSpellStaffLevel(item._iSpell);
-	r = GenerateRnd(player.getCharacterLevel() / r) + 1;
+	int rechargeStrength = RandomIntBetween(1, player.getCharacterLevel() / GetSpellStaffLevel(item._iSpell));
 
 	do {
 		item._iMaxCharges--;
 		if (item._iMaxCharges == 0) {
 			return;
 		}
-		item._iCharges += r;
+		item._iCharges += rechargeStrength;
 	} while (item._iCharges < item._iMaxCharges);
 
 	item._iCharges = std::min(item._iCharges, item._iMaxCharges);
@@ -4923,12 +4921,12 @@ bool ApplyOilToItem(Item &item, Player &player)
 	switch (player._pOilType) {
 	case IMISC_OILACC:
 		if (item._iPLToHit < 50) {
-			item._iPLToHit += GenerateRnd(2) + 1;
+			item._iPLToHit += RandomIntBetween(1, 2);
 		}
 		break;
 	case IMISC_OILMAST:
 		if (item._iPLToHit < 100) {
-			item._iPLToHit += GenerateRnd(3) + 3;
+			item._iPLToHit += RandomIntBetween(3, 5);
 		}
 		break;
 	case IMISC_OILSHARP:
@@ -4943,7 +4941,7 @@ bool ApplyOilToItem(Item &item, Player &player)
 		}
 		break;
 	case IMISC_OILSKILL:
-		r = GenerateRnd(6) + 5;
+		r = RandomIntBetween(5, 10);
 		item._iMinStr = std::max(0, item._iMinStr - r);
 		item._iMinMag = std::max(0, item._iMinMag - r);
 		item._iMinDex = std::max(0, item._iMinDex - r);
@@ -4964,7 +4962,7 @@ bool ApplyOilToItem(Item &item, Player &player)
 		break;
 	case IMISC_OILFORT:
 		if (item._iMaxDur != DUR_INDESTRUCTIBLE && item._iMaxDur < 200) {
-			r = GenerateRnd(41) + 10;
+			r = RandomIntBetween(10, 50);
 			item._iMaxDur += r;
 			item._iDurability += r;
 		}
@@ -4975,12 +4973,12 @@ bool ApplyOilToItem(Item &item, Player &player)
 		break;
 	case IMISC_OILHARD:
 		if (item._iAC < 60) {
-			item._iAC += GenerateRnd(2) + 1;
+			item._iAC += RandomIntBetween(1, 2);
 		}
 		break;
 	case IMISC_OILIMP:
 		if (item._iAC < 120) {
-			item._iAC += GenerateRnd(3) + 3;
+			item._iAC += RandomIntBetween(3, 5);
 		}
 		break;
 	default:

Source/levels/drlg_l1.cpp

@@ -372,7 +372,7 @@ void FillFloor()
 			if (dungeon[i][j] != Floor || Protected.test(i, j))
 				continue;
 
-			int rv = GenerateRnd(3);
+			int rv = RandomIntLessThan(3);
 			if (rv == 1)
 				dungeon[i][j] = Floor22;
 			else if (rv == 2)

Source/levels/drlg_l3.cpp

@@ -757,8 +757,8 @@ void CreateBlock(int x, int y, int obs, int dir)
 	int x2;
 	int y2;
 
-	int blksizex = GenerateRnd(2) + 3;
-	int blksizey = GenerateRnd(2) + 3;
+	int blksizex = RandomIntBetween(3, 4);
+	int blksizey = RandomIntBetween(3, 4);
 
 	if (dir == 0) {
 		y2 = y - 1;
@@ -1102,10 +1102,10 @@ void River()
 				if (dungeon[rx][ry] == 7) {
 					dircheck = 0;
 					if (dir < 2) {
-						river[2][riveramt] = GenerateRnd(2) + 17;
+						river[2][riveramt] = PickRandomlyAmong({ 17, 18 });
 					}
 					if (dir > 1) {
-						river[2][riveramt] = GenerateRnd(2) + 15;
+						river[2][riveramt] = PickRandomlyAmong({ 15, 16 });
 					}
 					river[0][riveramt] = rx;
 					river[1][riveramt] = ry;

Source/levels/drlg_l4.cpp

@@ -846,7 +846,7 @@ void Substitution()
 			if (FlipCoin(10)) {
 				uint8_t c = dungeon[x][y];
 				if (L4BTYPES[c] == 6 && !Protected.test(x, y)) {
-					dungeon[x][y] = GenerateRnd(3) + 95;
+					dungeon[x][y] = PickRandomlyAmong({ 95, 96, 97 });
 				}
 			}
 		}

Source/levels/town.cpp

@@ -221,7 +221,7 @@ void DrlgTPass3()
 		dungeon[16][35] = 7;
 		dungeon[17][35] = 7;
 		for (int x = 36; x < 46; x++) {
-			FillTile(x, 78, GenerateRnd(4) + 1);
+			FillTile(x, 78, PickRandomlyAmong({ 1, 2, 3, 4 }));
 		}
 	}
 	if (gbIsHellfire) {

Source/missiles.cpp

@@ -209,7 +209,7 @@ bool MonsterMHit(int pnum, int monsterId, int mindam, int maxdam, int dist, Miss
 	if (!monster.isPossibleToHit() || monster.isImmune(t, damageType))
 		return false;
 
-	int hit = GenerateRnd(100);
+	int hit = RandomIntLessThan(100);
 	int hper = 0;
 	const Player &player = Players[pnum];
 	const MissileData &missileData = GetMissileData(t);
@@ -1268,7 +1268,7 @@ void AddHorkSpawn(Missile &missile, AddMissileParameter &parameter)
 void AddJester(Missile &missile, AddMissileParameter &parameter)
 {
 	MissileID spell = MissileID::Firebolt;
-	switch (GenerateRnd(10)) {
+	switch (RandomIntLessThan(10)) {
 	case 0:
 	case 1:
 		spell = MissileID::Firebolt;
@@ -1497,7 +1497,7 @@ void AddWarp(Missile &missile, AddMissileParameter &parameter)
 void AddLightningWall(Missile &missile, AddMissileParameter &parameter)
 {
 	UpdateMissileVelocity(missile, parameter.dst, 16);
-	missile._miAnimFrame = GenerateRnd(8) + 1;
+	missile._miAnimFrame = RandomIntBetween(1, 8);
 	missile._mirange = 255 * (missile._mispllvl + 1);
 	switch (missile.sourceType()) {
 	case MissileSource::Trap:
@@ -1555,7 +1555,7 @@ void AddLightningBow(Missile &missile, AddMissileParameter &parameter)
 		dst += parameter.midir;
 	}
 	UpdateMissileVelocity(missile, dst, 32);
-	missile._miAnimFrame = GenerateRnd(8) + 1;
+	missile._miAnimFrame = RandomIntBetween(1, 8);
 	missile._mirange = 255;
 	if (missile._misource < 0) {
 		missile.var1 = missile.position.start.x;
@@ -1573,7 +1573,7 @@ void AddMana(Missile &missile, AddMissileParameter & /*parameter*/)
 
 	int manaAmount = (GenerateRnd(10) + 1) << 6;
 	for (int i = 0; i < player.getCharacterLevel(); i++) {
-		manaAmount += (GenerateRnd(4) + 1) << 6;
+		manaAmount += RandomIntBetween(1, 4) << 6;
 	}
 	for (int i = 0; i < missile._mispllvl; i++) {
 		manaAmount += (GenerateRnd(6) + 1) << 6;
@@ -1643,7 +1643,7 @@ void AddChargedBoltBow(Missile &missile, AddMissileParameter &parameter)
 	if (missile.position.start == dst) {
 		dst += parameter.midir;
 	}
-	missile._miAnimFrame = GenerateRnd(8) + 1;
+	missile._miAnimFrame = RandomIntBetween(1, 8);
 	missile._mlid = AddLight(missile.position.start, 5);
 	UpdateMissileVelocity(missile, dst, 8);
 	missile.var1 = 5;
@@ -1699,7 +1699,7 @@ void AddArrow(Missile &missile, AddMissileParameter &parameter)
 		const Player &player = Players[missile._misource];
 
 		if (HasAnyOf(player._pIFlags, ItemSpecialEffect::RandomArrowVelocity)) {
-			av = GenerateRnd(32) + 16;
+			av = RandomIntBetween(16, 47);
 		}
 		if (player._pClass == HeroClass::Rogue)
 			av += (player.getCharacterLevel() - 1) / 4;
@@ -1863,7 +1863,7 @@ void AddTeleport(Missile &missile, AddMissileParameter &parameter)
 void AddNovaBall(Missile &missile, AddMissileParameter &parameter)
 {
 	UpdateMissileVelocity(missile, parameter.dst, 16);
-	missile._miAnimFrame = GenerateRnd(8) + 1;
+	missile._miAnimFrame = RandomIntBetween(1, 8);
 	missile._mirange = 255;
 	const WorldTilePosition position = missile._misource < 0 ? missile.position.start : Players[missile._misource].position.tile;
 	missile.var1 = position.x;
@@ -1913,7 +1913,7 @@ void AddLightningControl(Missile &missile, AddMissileParameter &parameter)
 	missile.var1 = missile.position.start.x;
 	missile.var2 = missile.position.start.y;
 	UpdateMissileVelocity(missile, parameter.dst, 32);
-	missile._miAnimFrame = GenerateRnd(8) + 1;
+	missile._miAnimFrame = RandomIntBetween(1, 8);
 	missile._mirange = 256;
 }
 
@@ -1923,7 +1923,7 @@ void AddLightning(Missile &missile, AddMissileParameter &parameter)
 
 	SyncPositionWithParent(missile, parameter);
 
-	missile._miAnimFrame = GenerateRnd(8) + 1;
+	missile._miAnimFrame = RandomIntBetween(1, 8);
 
 	if (missile._micaster == TARGET_PLAYERS || missile.IsTrap()) {
 		if (missile.IsTrap() || Monsters[missile._misource].type().type == MT_FAMILIAR)
@@ -2611,7 +2611,7 @@ void AddChargedBolt(Missile &missile, AddMissileParameter &parameter)
 	if (missile.position.start == dst) {
 		dst += parameter.midir;
 	}
-	missile._miAnimFrame = GenerateRnd(8) + 1;
+	missile._miAnimFrame = RandomIntBetween(1, 8);
 	missile._mlid = AddLight(missile.position.start, 5);
 
 	UpdateMissileVelocity(missile, dst, 8);

Source/towners.cpp

@@ -701,19 +701,7 @@ void TalkToCowFarmer(Player &player, Towner &cowFarmer)
 		break;
 	case QUEST_HIVE_ACTIVE:
 		if (!player._pLvlVisited[9] && player.getCharacterLevel() < 15) {
-			_speech_id qt = TEXT_JERSEY12;
-			switch (GenerateRnd(4)) {
-			case 0:
-				qt = TEXT_JERSEY9;
-				break;
-			case 1:
-				qt = TEXT_JERSEY10;
-				break;
-			case 2:
-				qt = TEXT_JERSEY11;
-				break;
-			}
-			InitQTextMsg(qt);
+			InitQTextMsg(PickRandomlyAmong({ TEXT_JERSEY9, TEXT_JERSEY10, TEXT_JERSEY11, TEXT_JERSEY12 }));
 			break;
 		}
 

test/random_test.cpp

@@ -279,6 +279,31 @@ TEST(RandomTest, NegativeReturnValues)
 		EXPECT_EQ(GenerateRnd(i), -8) << "Unexpected return value for a limit of " << i;
 	}
 
+	// The following values are/were used throughout the code
+	SetRndSeed(1457187811);
+	EXPECT_EQ(GenerateRnd(37), -23) << "Unexpected return value for a limit of 37";
+
+	SetRndSeed(1457187811);
+	EXPECT_EQ(GenerateRnd(38), -12) << "Unexpected return value for a limit of 38";
+
+	SetRndSeed(1457187811);
+	// commonly used for dungeon megatile coordinates
+	EXPECT_EQ(GenerateRnd(40), -8) << "Unexpected return value for a limit of 40";
+
+	SetRndSeed(1457187811);
+	EXPECT_EQ(GenerateRnd(41), -9) << "Unexpected return value for a limit of 41";
+
+	SetRndSeed(1457187811);
+	EXPECT_EQ(GenerateRnd(52), -8) << "Unexpected return value for a limit of 52";
+
+	SetRndSeed(1457187811);
+	// commonly used for dungeon tile coordinates
+	EXPECT_EQ(GenerateRnd(80), -48) << "Unexpected return value for a limit of 80";
+
+	SetRndSeed(1457187811);
+	// commonly used for percentage rolls (typically in a context that 0 and -68 lead to the same outcome)
+	EXPECT_EQ(GenerateRnd(100), -68) << "Unexpected return value for a limit of 100";
+
 	for (int i = 1; i < 32768; i *= 2) {
 		SetRndSeed(1457187811);
 		EXPECT_EQ(GenerateRnd(i), 0) << "Expect powers of 2 such as " << i << " to cleanly divide the int_min RNG value ";