Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

I have a few comments on this.

Don't attempt to nest templates with the same parameter

First, as you've already discovered, nested templates with the same parameter are a violation of the C++ language standard, whether or not your compiler complains. (In fact, I would say it's broken if it does not complain.)

###C++ standard, section 14.6.1:

C++ standard, section 14.6.1:

A template-parameter shall not be redeclared within its scope (including nested scopes). A template-parameter shall not have the same name as the template name.

The definition of mUniformDistribution can be simpler

The code doesn't need a using statement. The declaration can be made directly:

decltype(dist<T>()) mUniformDistribution;

(Also note that the existing code misspells "Uniform")

The use of Random does not require a scope

The code in main refers to ::Random but that's really not necessary here. It should just be Random.

Consider repeatability

To answer the question in your code, yes,

std::default_random_engine mEngine{ std::random_device()() };

that line of code does seed mEngine however, I'd suggest rethinking that feature. If you were using this random number generator for, say a simulation, you might want to be able to duplicate the simulation exactly and not simply always run it with new random numbers. To get the best of both worlds, you could keep your existing code, but eliminate the initialization of mEngine and add it instead to the constructors:

Random(const T& min, const T& max)
 : mEngine{}, 
 mUniformDistribution(min, max)
{}
Random(const T& min, const T& max, bool )
 : mEngine{ std::random_device()() }, 
 mUniformDistribution(min, max)
{}
// ...
std::default_random_engine mEngine;

Now if you construct using the first version, you'll get the default seed each time. If you use the second constructor (with a dummy bool parameter) it will use a new seed each time.

I have a few comments on this.

Don't attempt to nest templates with the same parameter

First, as you've already discovered, nested templates with the same parameter are a violation of the C++ language standard, whether or not your compiler complains. (In fact, I would say it's broken if it does not complain.)

###C++ standard, section 14.6.1:

A template-parameter shall not be redeclared within its scope (including nested scopes). A template-parameter shall not have the same name as the template name.

The definition of mUniformDistribution can be simpler

The code doesn't need a using statement. The declaration can be made directly:

decltype(dist<T>()) mUniformDistribution;

(Also note that the existing code misspells "Uniform")

The use of Random does not require a scope

The code in main refers to ::Random but that's really not necessary here. It should just be Random.

Consider repeatability

To answer the question in your code, yes,

std::default_random_engine mEngine{ std::random_device()() };

that line of code does seed mEngine however, I'd suggest rethinking that feature. If you were using this random number generator for, say a simulation, you might want to be able to duplicate the simulation exactly and not simply always run it with new random numbers. To get the best of both worlds, you could keep your existing code, but eliminate the initialization of mEngine and add it instead to the constructors:

Random(const T& min, const T& max)
 : mEngine{}, 
 mUniformDistribution(min, max)
{}
Random(const T& min, const T& max, bool )
 : mEngine{ std::random_device()() }, 
 mUniformDistribution(min, max)
{}
// ...
std::default_random_engine mEngine;

Now if you construct using the first version, you'll get the default seed each time. If you use the second constructor (with a dummy bool parameter) it will use a new seed each time.

I have a few comments on this.

Don't attempt to nest templates with the same parameter

First, as you've already discovered, nested templates with the same parameter are a violation of the C++ language standard, whether or not your compiler complains. (In fact, I would say it's broken if it does not complain.)

C++ standard, section 14.6.1:

A template-parameter shall not be redeclared within its scope (including nested scopes). A template-parameter shall not have the same name as the template name.

The definition of mUniformDistribution can be simpler

The code doesn't need a using statement. The declaration can be made directly:

decltype(dist<T>()) mUniformDistribution;

(Also note that the existing code misspells "Uniform")

The use of Random does not require a scope

The code in main refers to ::Random but that's really not necessary here. It should just be Random.

Consider repeatability

To answer the question in your code, yes,

std::default_random_engine mEngine{ std::random_device()() };

that line of code does seed mEngine however, I'd suggest rethinking that feature. If you were using this random number generator for, say a simulation, you might want to be able to duplicate the simulation exactly and not simply always run it with new random numbers. To get the best of both worlds, you could keep your existing code, but eliminate the initialization of mEngine and add it instead to the constructors:

Random(const T& min, const T& max)
 : mEngine{}, 
 mUniformDistribution(min, max)
{}
Random(const T& min, const T& max, bool )
 : mEngine{ std::random_device()() }, 
 mUniformDistribution(min, max)
{}
// ...
std::default_random_engine mEngine;

Now if you construct using the first version, you'll get the default seed each time. If you use the second constructor (with a dummy bool parameter) it will use a new seed each time.

Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

I have a few comments on this.

Don't attempt to nest templates with the same parameter

First, as you've already discovered, nested templates with the same parameter are a violation of the C++ language standard, whether or not your compiler complains. (In fact, I would say it's broken if it does not complain.)

###C++ standard, section 14.6.1:

A template-parameter shall not be redeclared within its scope (including nested scopes). A template-parameter shall not have the same name as the template name.

The definition of mUniformDistribution can be simpler

The code doesn't need a using statement. The declaration can be made directly:

decltype(dist<T>()) mUniformDistribution;

(Also note that the existing code misspells "Uniform")

The use of Random does not require a scope

The code in main refers to ::Random but that's really not necessary here. It should just be Random.

Consider repeatability

To answer the question in your code, yes,

std::default_random_engine mEngine{ std::random_device()() };

that line of code does seed mEngine however, I'd suggest rethinking that feature. If you were using this random number generator for, say a simulation, you might want to be able to duplicate the simulation exactly and not simply always run it with new random numbers. To get the best of both worlds, you could keep your existing code, but eliminate the initialization of mEngine and add it instead to the constructors:

Random(const T& min, const T& max)
 : mEngine{}, 
 mUniformDistribution(min, max)
{}
Random(const T& min, const T& max, bool )
 : mEngine{ std::random_device()() }, 
 mUniformDistribution(min, max)
{}
// ...
std::default_random_engine mEngine;

Now if you construct using the first version, you'll get the default seed each time. If you use the second constructor (with a dummy bool parameter) it will use a new seed each time.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /