convert GCCRandom(Mersenne Twister by Takuji Nishimura and Makoto Matsumoto) to stl random

I am trying to translate the book Game Coding Complete by Mike McShaffry 4E to modern C++17 standard and faced with the code of Mersenne Twister by Takuji Nishimura and Makoto Matsumoto. Is it right to convert random generator code from book like that:

from:

#include <ctime>
#define CMATH_N 624
#define CMATH_M 397
#define CMATH_MATRIX_A 0x9908b0df   /* constant vector a */
#define CMATH_UPPER_MASK 0x80000000 /* most significant w-r bits */
#define CMATH_LOWER_MASK 0x7fffffff /* least significant r bits */
#define CMATH_TEMPERING_SHIFT_U(y)  (y >> 11)
#define CMATH_TEMPERING_SHIFT_S(y)  (y << 7)
#define CMATH_TEMPERING_SHIFT_T(y)  (y << 15)
#define CMATH_TEMPERING_SHIFT_L(y)  (y >> 18)
typedef int INT;
typedef unsigned int UINT;
#define MAXUINT ((UINT)~((UINT)0))
#define MAXINT ((INT)(MAXUINT >> 1))
class GCCRandom {
private:
unsigned int rseed;
unsigned int rseed_sp;
unsigned long mt[CMATH_N]; /* the array for the state vector  */
int mti; /* mti==N+1 means mt[N] is not initialized */
public:
GCCRandom() {
rseed = 1;
rseed_sp = 0;
mti = CMATH_N + 1;
}
unsigned int Random(unsigned int n) {
unsigned long y;
static unsigned long mag01[2] = { 0x0, CMATH_MATRIX_A };
if (n == 0)
return(0);
if (mti >= CMATH_N) { /* generate N words at one time */
int kk;
if (mti == CMATH_N + 1)   /* if sgenrand() has not been called, */
SetRandomSeed(4357); /* a default initial seed is used   */
for (kk = 0; kk < CMATH_N - CMATH_M; kk++) {
mt[kk] = mt[kk + CMATH_M] ^ (y >> 1) ^ mag01[y & 0x1];
}
for (; kk < CMATH_N - 1; kk++) {
mt[kk] = mt[kk + (CMATH_M - CMATH_N)] ^ (y >> 1) ^ mag01[y & 0x1];
}
mt[CMATH_N - 1] = mt[CMATH_M - 1] ^ (y >> 1) ^ mag01[y & 0x1];
mti = 0;
}
y = mt[mti++];
y ^= CMATH_TEMPERING_SHIFT_U(y);
y ^= CMATH_TEMPERING_SHIFT_L(y);
return (y % n);
}
float Random() {
float r = (float)Random(MAXINT);
float divisor = (float)MAXINT;
return (r / divisor);
}
void SetRandomSeed(unsigned int n) {
/* setting initial seeds to mt[N] using         */
/* the generator Line 25 of Table 1 in          */
/* [KNUTH 1981, The Art of Computer Programming */
/*    Vol. 2 (2nd Ed.), pp102]                  */
mt[0] = n & 0xffffffff;
for (mti = 1; mti < CMATH_N; mti++)
mt[mti] = (69069 * mt[mti - 1]) & 0xffffffff;
rseed = n;
}
unsigned int GetRandomSeed() {
return rseed;
}
void Randomize() {
SetRandomSeed((unsigned int)time(NULL));
}
};


to code with std::mt19937 engine and std::uniform_int_distribution:

#include <random>
#include <limits>
#include <chrono>
class MTRandom {
private:
unsigned int m_rseed = 1;
unsigned int m_rseed_sp = 0;
static const unsigned int MAXINT = std::numeric_limits<int>::max();
std::mt19937 m_engine;
public:
MTRandom() { m_engine.seed(m_rseed); }
unsigned int Random(unsigned int n) {
std::uniform_int_distribution<unsigned int> distr(0, n);
return distr(m_engine);
};
float Random() {
std::uniform_real_distribution<float> distr(0.0f, 1.0f);
return distr(m_engine);
}
void SetRandomSeed(unsigned int n) {
m_engine.seed(n);
m_rseed = n;
}
unsigned int GetRandomSeed() {
return m_rseed;
}
void Randomize() {
SetRandomSeed((unsigned int)std::chrono::system_clock::to_time_t(std::chrono::system_clock::now()));
}
};


The short answer is (mostly) yes. Your use of C++17 and std::mt19937 looks mostly correct.

However:

• You have std::uniform_int_distribution<unsigned int> distr(0, n), and this generates random values from 0 to n inclusive. Your original code appears to generate values from 0 to n-1 inclusive. So you should use std::uniform_int_distribution<unsigned int> distr(0, n-1). In addition, before doing that, you would need to check whether n equals zero:

unsigned int Random(unsigned int n) {
if (n == 0)
return 0;
std::uniform_int_distribution<unsigned int> distr(0, n-1);
return distr(m_engine);
};

• Your code declares m_rseed_sp and MAXINT, both of which are not used. It would be better to remove them. Then you can also remove #include <limits>.

• It is probably slightly more efficient to create member variables for std::uniform_int_distribution and for std::uniform_real_distribution. For std::uniform_real_distribution this is trivial because you never change the parameters of the distribution. For std::uniform_int_distribution, it is slightly more complicated because now you have to create parameters and pass them to the distribution. Something like this (untested):

private:
using IntDistribution = std::uniform_int_distribution<unsigned int>;
IntDistribution m_int_distribution;

public:
unsigned int Random(unsigned int n) {
if (n == 0)
return 0;
IntDistribution::param_type parameters{0, n-1};
return m_int_distribution(m_engine, parameters);
};

• For the Randomize() function, you might consider something like this instead:

void Randomize() {
std::random_device randomDevice;
SetRandomSeed(randomDevice());
}


Ideally, this seems better than just using the current time. However, I've seen rumors that std::random_device is not implemented well on a few platforms, so maybe what you have is better? That's a judgement call.

• Note: There is no guarantee that std::mt19937 will initialize its internal state identically to the way your old code did, in fact I think it is very unlikely. So you can't expect identical output even if you use the same seed.