Multithreading C++ loop

I've made a small skeleton for a larger project that will include cross-platform multithreading (by using Boost) and thread-safe random numbers (by using GNU scientific libraries and mutexes). My goal is to run a simulation (probably over 100,000 loops) in which each thread will utilize a random number and read from a library that the threads will not change the value of (thus, I don't bother putting the library in a mutex)!

Is this code alright?

```
#include // Standard in out for c
#include // Vectors
#include // Boost threads
#include // Boost mutexes
#include // Gnu Scientific Library, thread safe rand

const gsl_rng_type * T = gsl_rng_default; // Select random generator
gsl_rng * r = gsl_rng_alloc (T); // Allocate the range
boost::mutex mtx; // Create a mutex
std::vector data; // Create a vector

int count=23; // Have a total number of threadFunctions you want to loop through
int CORES = static_cast(boost::thread::hardware_concurrency());

void threadFunction(int id);

int main()
{
printf("Number of cores = %d\n-------------------\n\n",CORES);
gsl_rng_env_setup(); // Setup the random variable environment
gsl_rng_set (r,time(NULL)); // Seed the r generator
std::vector t; // Create vector of boost::thread pointers
while(count>0){ // While you are looping
for(int i=0;icount?count:CORES));i++) // Create 4 threads if more than 4 loops are left, else create the appropriate number of threads
t.push_back(new boost::thread(threadFunction,i)); // Push threads objects back
for(int i=0;ijoin(); // Wait to join. Note, these are pointers so use -> instead of .
count--; // Reduce count by 1
}
t.resize(0); // Empty the array
}

printf("\nEnd of threads...");
FILE *out = fopen("RandomNumbers.txt","w+"); // Create a file to hold values
for(int i=0;i ownlock(mtx); // Create lock_guard
data.push_back(gsl_rng_get(r)); // Put a random number in data
}
printf("Working

-
You're overdoing it with comments. A lot. And trivial ones at that. Every programmer knows that std::vector t; creates a vector of pointers to boost::thread, or that you have to use -> with pointers. Such comments are really just clutter and actually make the code harder to read. Save them for the non-obvious bits.

-
It's unclear what mtx protects. Does it protect data? Or r? Or both? This is one of the places where you should comment, but you didn't (//Create a mutex is absolutely pointless). If possible, rename the mutex to match its purpose. It would also be a good idea to wrap the mutex and the data it protects in a class, to ensure that unsafe access is impossible.

-
((CORES>count?count:CORES)) requires the reader to parse it (and what's with the double parentheses?) The meaning of std::min(CORES, count), on the other hand, is immediately obvious. And how does it create 4 threads? You could just as well rewrite the entire loop to make it more concise (and perhaps more efficient too):

while (count > 0) {
  std::vector threads(std::min(CORES, count));
  for (size_t i = 0; i join();
    delete threads[i];
    --count;
  }
}

Also note that I added a call to delete into the waiting loop. Your previous code was leaking the threads.

-
You're writing C++. Don't use (type unsafe!) C I/O. Do this instead:

{
  std::ofstream out("RandomNumbers.txt");
  for (size_t idx = 0; idx < data.size(); ++idx) {
    out << data[idx] << '\n';
  }
}

The same holds for all other uses of C I/O (printf(), getchar()), of course.

-
count and CORES would be better declared with type size_t, as they represent the number of some objects.

-
If you have access to C++11, all the above could be simplified even further using range-based for loops, lambdas in standard algorithms, or at the very least iterator-based loops. I didn't bother with iterators here (even though they could be used), because their type has to be spelled out explicitly.

In such case, it would also make more sense not to allocate the threads dynamically, but create them directly in the vector. No need for manual deallocation, no dynamic allocation overhead.

StackExchange Code Review Q#32817, answer score: 8