C++ <random> convenience functions

Question

I have a set of functions in my program for conveniently generating random number in various distributions. I'm interested in the questions listed below, though other comments are also welcome.

1. Am I using C++'s random tools correctly and clearly?
2. From watching rand() Considered Harmful and reading this answer, I learned that RNG calls are not thread-safe. I changed my code just by adding the specifier thread_local to the generator and distribution variables. Is this all that's needed to make the functions thread-safe? These are called within std::async procedures in my project.
3. Does it matter whether one std::mt19937_64 generator is used for all the functions or if each function has its own copy?

In Random.h

#ifndef RANDOM_H
#define RANDOM_H

#include <random>
#include <algorithm>

namespace Random
{
    // Random number with normal distribution and mean of zero
    double random_normal(double standard_deviation);

    // Random number with inclusive range
    double random_real(double min, double max);
    int random_integer(int min, int max);

    // Return true with probability 50%
    bool coin_flip();

    // Return true with given probability
    bool success_probability(double probability);

    namespace
    {
        thread_local std::mt19937_64 generator(std::random_device{}());
    }

    // Shuffles the order of the list
    template<class List>
    void shuffle(List& list)
    {
        std::shuffle(list.begin(), list.end(), generator);
    }
}
#endif // RANDOM_H

In Random.cpp

#include "Random.h"

#include <random>

int Random::random_integer(int min, int max)
{
    using uid = std::uniform_int_distribution<int>;
    thread_local static auto dist = uid{};
    return dist(generator, uid::param_type{min, max});
}

double Random::random_normal(double standard_deviation)
{
    using nd = std::normal_distribution<double>;
    thread_local static auto dist = nd{};
    return dist(generator, nd::param_type{0.0, standard_deviation});
}

double Random::random_real(double min, double max)
{
    using urd = std::uniform_real_distribution<double>;
    thread_local static auto dist = urd{};
    return dist(generator, urd::param_type{min, max});
}

bool Random::coin_flip()
{
    return success_probability(0.5);
}

bool Random::success_probability(double probability)
{
    return random_real(0.0, 1.0) < probability;
}

An example usage:

#include <iostream>
#include <future>

#include "Random.h"

double random_walk(int number_of_steps, double step_size)
{
    double position = 0;
    for(int i = 0; i < number_of_steps; ++i)
    {
        position += Random::random_normal(step_size);
    }
    return position;
}

int main()
{
    // Simulate 1,000,000-step continuous random walks
    const int number_of_walks = 100; // adjust to suit your computer
    const int number_of_steps = 1000000;
    const double step_size = 1.0;
    std::vector<std::future<double>> results;
    for(int i = 0; i < number_of_walks; ++i)
    {
        results.emplace_back(std::async(random_walk, number_of_steps, step_size));
    }

    for(auto& result : results)
    {
        std::cout << result.get() << std::endl;;
    }

    return 0;
}

Answer 1

To answer your questions:

1. Sure looks like it.
2. No, see next point.
3. No, it is not safe. std::mersenne_twister_engine::operator() reads and increments its internal state. AFAIK, this makes it not thread safe at all.

So, here are a few suggestions:

1. An extra semicolon hid itself in your code:

   std::cout << result.get() << std::endl;;
                                          ^
   

2. Don't use std::endl. It implicitly uses std::flush to flush the stream. This is not very cheap, so it is better for performance if you just output '\n'.

   If you do want to flush the stream, it would be better to state your intent and use std::flush, but this depends on you.

3. Mark functions that do not throw noexcept. That way, you state your intent that the functions will not throw, and it helps the compiler optimize a bit.

4. Currently, your shuffle takes a container. While that is desirable in most cases, sometimes, you just need to shuffle a subrange of a container. It's for exactly this reason that all the standard algorithms take an iterator pair instead of a container.

5. If you try to include Random.h more than once, you will get symbol collision for the variable generator. The same thing happens for functions. Here's what you can do:

   • Use C++17, and mark the variable inline, which behaves exactly like in functions:

     thread_local inline std::mt19937_64 generator(std::random_device{}());
     

   • Mark the variable extern, and put its definition in Random.cpp.

6. You don't need return 0; at the end of main(). The compiler implicitly adds it for you.

7. I like to use types as parameter that try to match the preconditions of a function. For example, I would use unsigned integers everywhere where you use signed integers, because I do not see the point in calling let's say random_walk with a negative number_of_steps.

8. You can shorten the definitions in Random.cpp by removing Random:: if you wrap them around namespace Random, like in the declarations.

Answer 2

I would strongly suggest, that you create convenience classes for that use case, which act similar to functions.

The reason is, that the random number generators generally increment their state and more importantly that initialization is costly. Therefore, create a class that holds the generator and the distribution and create an operator()

#include <random>

class randomStreamNormal {
public:
    explicit randomStreamNormal(double mean, double stddev)
    : mt(rand()), norm_dist(mean, stddev) {}
    explicit randomStreamNormal(double mean, double stddev, double seed)
    : mt(seed), norm_dist(mean, stddev) {}

    double operator ()(void) { return norm_dist(mt); }
private:
    std::mt19937_64                     mt;
    std::normal_distribution<double>    norm_dist;
};

class randomStreamUniformInt {
public:
    explicit randomStreamUniformInt(int lower_bound, int upper_bound)
    : mt(rand()), uniform_dist(lower_bound, upper_bound) {}
    explicit randomStreamUniformInt(int lower_bound, int upper_bound, double seed)
    : mt(seed), uniform_dist(lower_bound, upper_bound) {}

    int operator ()(void) { return uniform_dist(mt); }
private:
    std::mt19937_64                     mt;
    std::uniform_int_distribution<>     uniform_dist;
};

Noter that i used rand() because I needed to be able to reproduce the results.