I see some things that may help you improve your program.
Don't abuse using namespace std
Putting using namespace std
at the top of every program is a bad habit that you'd do well to avoid.
Eliminate global variables where practical
The code declares and uses 5 global variables. Global variables obfuscate the actual dependencies within code and make maintainance and understanding of the code that much more difficult. It also makes the code harder to reuse. For all of these reasons, it's generally far preferable to eliminate global variables and to instead pass pointers to them. That way the linkage is explicit and may be altered more easily if needed. For example, one way to do it would be to gather all of the variables into a struct
and pass a reference to the struct to each thread instance. The structure instance could be a local variable within main
.
Think about eliminating redundant variables
Since boolean variables evenready
and oddready
are always in opposite states, one of them is redundant. In fact, in this case, both are redundant since one can easily derive the same function from the value of x
.
Use appropriate C++ idioms
This line is somewhat strange:
for (; x < 10;) {
It's much more clear to write like this:
while (x < 10) {
Omit return 0
When a C++ program reaches the end of main
the compiler will automatically generate code to return 0, so there is no need to put return 0;
explicitly at the end of main
.
Don't Repeat Yourself (DRY)
If you're writing almost identical functions, think if there's a way to consolidate them. In this case there certainly is, as I'll demonstrate later in this answer.
Think carefully about data race conditions
A mutex
is generally used to assure non-conflicting access to a shared resource. For that reason, it's good to clearly answer the question, "what shared resource is this mutex
protecting?" In this case, it seems to be protecting access to std::cout
and x
but it doesn't do a thorough job of that. Consider that when one thread is evaluating x < 10
(without a lock) the other might be incrementing x
(with a lock). That's a classic data race. Here's a rewrite that avoids this problem:
#include <iostream>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <functional>
#include <string_view>
struct OddEven {
int x = 1;
std::mutex m;
std::condition_variable cond;
};
void printTask(OddEven &oe, const std::string_view &label, bool odd)
{
for (bool running{true}; running; ) {
std::unique_lock<std::mutex> mlock(oe.m);
oe.cond.wait(mlock, [&oe, odd] {
return (oe.x & 1) == odd;
});
std::cout << label << oe.x << std::endl;
oe.x++;
running = oe.x < 10;
oe.cond.notify_all();
}
}
int main()
{
OddEven oe;
std::thread t1(printTask, std::ref(oe), "Odd Print", true);
std::thread t2(printTask, std::ref(oe), "Even Print", false);
t1.join();
t2.join();
}