8
\$\begingroup\$

Based on this question, I have made a simple program to print a sequence of random numbers. The program seems to run fine, but I would like to know if my implementation is correct and how I can improve it.

#include <random>
#include <array>
#include <algorithm>
#include <functional>
#include <iostream>

template<class T = std::mt19937, std::size_t N = T::state_size>
auto randomEngine() -> typename std::enable_if_t<!!N, T>
{
    std::array<typename T::result_type, N> seed_data;
    thread_local static std::random_device source;
    std::generate(std::begin(seed_data), std::end(seed_data), std::ref(source));
    std::seed_seq seeds(std::begin(seed_data), std::end(seed_data));
    thread_local static T seeded_engine(seeds);
    return seeded_engine;
}

template<typename T>
T random(T min, T max)
{
    static_assert(std::is_integral<T>::value || std::is_floating_point<T>::value, "!");

    using UniformInt = std::uniform_int_distribution<T>;
    using UniformReal = std::uniform_real_distribution<T>;
    using DistType = std::conditional_t<std::is_integral<T>::value, UniformInt, UniformReal>;

    static auto RandomEngine = randomEngine();

    DistType uniformDistribution(min, max);

    return uniformDistribution(RandomEngine);
}

int main()
{
    for (auto i = 0u; i < 16; ++i) 
        std::cout << random(0u, 15u) << ' ';
}
\$\endgroup\$
10
\$\begingroup\$

Extra typenames

Every place you use typename, it's unnecessary. std::enable_if_t will take care of that for you.

thread_local

First of all, thread_local implies static, so writing both is redundant. Secondly, if you're making it thread_local, that's effectively like making a thread-specific singlelton - so you should return a reference to it, not a copy:

template<class T = std::mt19937, std::size_t N = T::state_size>
auto randomEngine() -> std::enable_if_t<!!N, T&>

And take it by reference:

static auto& RandomEngine = randomEngine();

random()

I'm not sure it's a good idea to create the distribution every time. The distribution object itself can keep state, so I think it'd be a better idea to take it in instead:

template <class Dist>
typename Dist::result_type random(Dist& dist)
{
    static auto& RandomEngine = randomEngine();
    return dist(RandomEngine);
}

And alter dist<T> to take arguments so that you could do:

auto d = dist<unsigned>(0, 15);
for (auto i = 0u; i < 16; ++i) 
    std::cout << random(d) << ' ';   
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.