Dining philosophers problem in C++11

Is this implementation correct? Do you find any threading problem? Also, what parts of the code can be changed to be more C++11-ish?

```
#include
#include
#include
#include
#include

namespace {
int sleep_for_random_time() {
std::random_device rd;
std::mt19937 mt(rd());
std::uniform_real_distribution dist(1000, 4000);

return dist(mt);
}
}

class DiningPhilosopher {
public:
DiningPhilosopher(int numOfPhilosophers_ = 5, int numOfForks_ = 5, int numOfEating_ = 3) : numOfPhilosophers(numOfPhilosophers_), numOfForks(numOfForks_), numOfEating(numOfEating_) {
forks.push_back(new Fork);
forks.push_back(new Fork);
forks.push_back(new Fork);
forks.push_back(new Fork);
forks.push_back(new Fork);

numAllowed = numOfForks - 1;
}

~DiningPhilosopher() {
for(const auto& fork : forks) {
delete fork;
}
}

DiningPhilosopher(const DiningPhilosopher&) = delete;
DiningPhilosopher& operator = (const DiningPhilosopher&) = delete;

void StartDining() {
for(int i = 0; i lock(permission);
cv.wait(permission, [this] { return numAllowed > 0; });
--numAllowed;
}

void GrantPermission() {
std::lock_guard lock(permission);
++numAllowed;

if(numAllowed == 1) {
cv.notify_all();
}
}

void Think(int id) {
std::chrono::milliseconds duration(sleep_for_random_time());
std::this_thread::sleep_for(duration);
}

void Eat(int id) {
this->WaitForPermission();
forks[id]->mux.lock();
forks[(id + 1) % numOfForks]->mux.lock();

std::cout Think(id);
std::cout mux.unlock();
forks[(id + 1) % numOfForks]->mux.unlock();

this->GrantPermission();
}

void Philosopher(int id) {
for(int i = 0; i Think(id);
this->Eat(id);
}

std::cout threads;
std::vector forks;

struct Fork {
    std::mutex mux;
};
std::vector forks;

I think the problem you want to solve by using pointers here is that a std::mutex is not copyable nor movable. By using raw owning pointers, you'll get a bunch of unpleasant properties. For example, you have to implement a destructor, and might have to worry about exception safety.

Typically, using smart pointers instead of raw owning pointers is a better idea, since it avoids many of those unpleasant side-effects. But the original problem can be solved differently:

std::vector forks; // data member
DiningPhilosopher(..) // constructor
  : // ...
  , forks(5)
{}

This uses a vector constructor that constructs 5 std::mutex in-place. It therefore does not require that the value type is movable. You cannot resize such a vector, though, nor can you add new entries via push_back etc, even if there's space left (capacity > size). If you have a fixed number of forks, you could use a std::array as well.

If you need to add more forks later, or need to resize:

std::vector> forks;
DiningPhilosopher(..) // constructor
  : // ...
  , forks(5)
{
    for(auto& fork : forks) fork = std::make_unique();
}

make_unique is C++14, but can be easily implemented in C++11 as well.
It is also possible to use std::generate_n:

DiningPhilosopher(..) // constructor
  : // ...
{
    std::generate_n(std::back_inserter(forks), 5
                    []{ return std::make_unique(); });
}

threads.push_back(std::thread(&DiningPhilosopher::Philosopher, this, i));

This can be replaced by the slightly simpler:

threads.emplace_back(&DiningPhilosopher::Philosopher, this, i);

Or even:

threads.push_back([this, i]{ Philosopher(i); });

std::for_each(threads.begin(),threads.end(), std::mem_fn(&std::thread::join));

alternative:

for(auto& t : threads) t.join();

One might argue about the use of "raw loops", but you don't need the position of each thread (the iterator) here. Therefore, I'd say that a for-each loop is fine as well; and it's shorter.

void Philosopher(int id)

The name of this function surprised me when I encountered &DiningPhilosopher::Philosopher earlier. It almost looked like a data member, since it's not formulated as a verb. From a code organization perspective, I think it should be earlier in the code, before the functions it calls. This allows a top-down reading from the higher to lower level functions.

std::cout Think(id);
 std::cout << "Philosopher "<< id << " has finished eating" << std::endl;

The philosopher thinks while they're eating? Ok, I think it is code reuse, but the name confused me. Why not introduce an intermediary function wait_for_random_time that is used to implement both thinking and (the actual) eating? Similarly, the function Eat doesn't just let a philosopher eat. It does more (waiting to eat etc.)

int sleep_for_random_time() {
    std::random_device rd;
    std::mt19937 mt(rd());
    std::uniform_real_distribution dist(1000, 4000);

    return dist(mt);
}

glampert has already criticised the name of this function in his answer, the fact that it repeatedly creates a random_device as well as a mt19937, and the usage of double.

I'd also like to point out that creating both generators seems strange to me: First you get a (potentially pseudo-)random number from one random number generator (the random_device). Then, you use it to seed another random number generator, to create a pseudo-random number. The second step is unnecessary, if you trust the quality of your random_device implementation enough: It either produces true random numbers, or - and that's the Quality Of Implementation aspect - evenly distributed pseudo-random numbers. The following should be sufficient:

int sleep_for_random_time() {
    std::random_device rd;
    std::uniform_int_distribution dist(1000, 4000);
    return dist(rd);
}

The original code followed a common pattern of creating pseudo-random numbers. Typically, one does not use the random device to create all random numbers because it is slow (it might run out of entropy, too, depending on the implementation). You'd therefore use it only once to initialize a single mersenne twister. Successive requests for pseudo-random numbers would use this single mersenne twister to produce pseudo-random numbers. For example:

int sleep_for_random_time() {
     static std::mt19937 mt{ std::random_device{}() };
     std::uniform_int_distribution dist(1000, 4000);
     return dist(mt);
 }

Arguably, even the distribution should be static, to allow recycling unused bits of the output of the generator. As Deduplicator points out, there could/should be one generator per thread. The above code uses one shared generator, which is not thread-safe, since dist(mt) modifies the state of the (now shared) generator. There are several ways to make this thread-safe, one of them is:

```
int sleep_for_ra

StackExchange Code Review Q#74978, answer score: 12