Return to Answer

• 1

• 97.7k
• 5
• 126
• 341

##Overall

Wow. Correct implementation.
Your the first person to post what I would call a proper implementation.

Good Job.

##Code Review

###Prefer NOT to use #define

#define P 5

Nearly every use of #define has a better alternative in C++ (unless you are really abstracting OS specific details).

In this case case you are defining a constant integer. There is an explicit version of that for C++.

constexpr int globalConstantP = 5; // More than 1 letter

While we are on P. A variable of length 1 is not very meaningful. Your code should be self documenting. This really means your variable names should be meaningful.

Avoid Platform specific includes.

#include <Windows.h>

This is the one place where #define comes in useful. Abstracting platfrom specific details. The Windows and Unix versions of sleep are different.

#if defiend(WIN32)
#include <Windows.h>
#define doSleep(X) Sleep(X)
#elif defined(__linux__)
#include <unistd.h>
#define doSleep(X) sleep(X * 1000)
#elif defined (__APPLE__) && defined (__MACH__)
#include <unistd.h>
#define doSleep(X) sleep(X * 1000)
#else
#error "Unsupported Platform"
#endif

Avoid using pointers

std::thread *threads[P];

In modern C++ you should never (or practically never) be calling new and delete. You should use automatic variables for all situations so that you can make sure the constructor/destructor and thus RAII kicks in to do the resource management. When you do need dynamically allocated objects they should be wrapped in smart pointers so that they can not leak.

Here you don't need pointers at all. Just use std::thread.

###Avoid global variables

std::thread *threads[P];
std::mutex mtx[P]; //forks

Global mutable state is the bane of many problems in program. But especially testing. It is best to pass objects to functions (by reference) and manipulate them or even better to just manipulate the object by calling the member functions.

I would have created a class called Table that had the array of std::thread and std::mutex objects. Then your functions below become methods than manipulate the object.

cout!!!

std::mutex cout;

That is too easy to be confused with std::cout. Please put it in a namespace and the very least. I would actually create your own stream class (one that can be locked) and pass that as the output object to the constructor of your Table class.

###Thinking should sleep

It take times to think.
It should also take time to think.

 if (pickUp(leftIndex, rightIndex)) {
 std::cout << "Philosopher " << philID << " eats.\n";
 doSleep(rand() % 20);
 // Don't put Down the forks until you have had time
 // time to do some eating. Remeber you are simulating work
 // after having obtained the resources. The work should
 // take none zero time.
 putDown(leftIndex, rightIndex);
 else {
 std::cout << "Philosopher " << philID << " thinks.\n";
 // When the philosopher is thinking the time should
 // also be none zero as you are simulating doing work
 // that is not related to the resource.
 doSleep(rand() % 20);
 }

Joinable

 if (threads[i - 1]->joinable())
 threads[i - 1]->join();

No need to test for joinable if you have never called detach on your thread.

Return 0

 return 0;

No need to return 0 at the end of main(). It does that automatically.

Normally you use return 0 to indicate there was a success BUT also to indicate that it could fail and there is another way to exit that could return a non zero value. When I see a return 0 I immediately start looking for alternative returns paths to see under what conditions the application will fail.

So if your application can not fail don't return anything (this is an indication that it can't fail). The compiler will add the return 0 if one was not provided.