A vector returns gibberish when a structure object is created before the user inserts data

Question

I train with pointers and stumbled upon something I don't understand. The program does the following:

• Create vector
• pass the vector address of the function
• This function has for loop
• In this loop, the movie name is requested for the user
• After receiving the name of the movie, a new movie object is created (from the structure)
• A new enhancement stream is created for each film, passing the title created by the user and a pointer for both the new movie object and the vector.
• In the accelerating stream, the variable "title" of the movie object is assigned the title that the user created, and then the movie is added to the vector
• When all threads are executed, the for loop inside the main loop shows all the movie names stored in the vector.

The problem arises when I replace these two

//Get info about new movie from user string movieTitle; int movieYear; //Not used at the moment cout << "Enter title for movie " << (i+1) << endl; getline(cin,movieTitle); 

and

 //Create new movie movies_t amovie; movies_t * pmovie; pmovie = &amovie; 

When I put the "user input part" over the "create a new movie" part, as of now, I get the following:

But when I exchange them:

I do not understand why, because they do not affect each other at all.

Data is displayed as follows:

 for(i=0;i<movieVector.size();i++) { cout << movieVector[i].title << endl; } 

These are the corresponding functions (main and newThreads)

 void newThreads(vector<movies_t> *movieVectorPointer) { boost::thread_group group; //Start thread group int i; for(i=0; i<2; i++) //Make X amount of threads (2 in this case) { //Get info about new movie from user string movieTitle; int movieYear; //Not used at the moment cout << "Enter title for movie " << (i+1) << endl; getline(cin,movieTitle); //Create new movie movies_t amovie; movies_t * pmovie; pmovie = &amovie; //Let user know we are now starting on this thread cout << "Doing thread " << i << endl; //Start new thread newThreadStruct startNewThread(movieTitle,movieYear,pmovie,movieVectorPointer); group.create_thread(startNewThread); } group.join_all(); //Wait for all threads in group to finish } int main() { cout << "Hello world!" << endl; //I am born. vector<movies_t> movieVector; //Create vector to store movies in newThreads(&movieVector); //Start making new threads. Pass movieVector address so it can be used within threads. /* The below code will not be executed until all threads are done */ cout << "Amount of movies " << movieVector.size() << endl; //Let user know how many movies we made //Show all movies we made int i; for(i=0;i<movieVector.size();i++) { cout << movieVector[i].title << endl; } cout << "Bye world!" << endl; //This life has passed. return 0; } 

And here is the full code, if that matters:

 #include <iostream> #include <boost/thread.hpp> using namespace std; //A movie will hold a title and a year. Only title is used in this code. struct movies_t { string title; int year; }; //This function is where the data is added to our new movie,a fter which our finished movie will be added to the vector. It is called from within the "newThreadStruct" operator. int doMovieWork(string movieTitle,int movieYear,movies_t *moviePointer,vector<movies_t> *movieVector) { moviePointer->title = movieTitle; //Add title to new movie movieVector->push_back(*moviePointer); //Add movie to vector return 0; }; //This struct will be used to create new Boost threads. It accepts various arguments. struct newThreadStruct { newThreadStruct(string movieTitle,int movieYear,movies_t *moviePointer,vector<movies_t> *movieVector) : movieTitle(movieTitle),movieYear(movieYear),moviePointer(moviePointer),movieVector(movieVector) { } void operator()() { doMovieWork(movieTitle,movieYear,moviePointer,movieVector); } string movieTitle; int movieYear; movies_t *moviePointer; vector<movies_t> *movieVector; }; void newThreads(vector<movies_t> *movieVectorPointer) { boost::thread_group group; //Start thread group int i; for(i=0; i<2; i++) //Make X amount of threads (2 in this case) { //Get info about new movie from user string movieTitle; int movieYear; //Not used at the moment cout << "Enter title for movie " << (i+1) << endl; getline(cin,movieTitle); //Create new movie movies_t amovie; movies_t * pmovie; pmovie = &amovie; //Let user know we are now starting on this thread cout << "Doing thread " << i << endl; //Start new thread newThreadStruct startNewThread(movieTitle,movieYear,pmovie,movieVectorPointer); group.create_thread(startNewThread); } group.join_all(); //Wait for all threads in group to finish } int main() { cout << "Hello world!" << endl; //I am born. vector<movies_t> movieVector; //Create vector to store movies in newThreads(&movieVector); //Start making new threads. Pass movieVector address so it can be used within threads. /* The below code will not be executed until all threads are done */ cout << "Amount of movies " << movieVector.size() << endl; //Let user know how many movies we made //Show all movies we made int i; for(i=0;i<movieVector.size();i++) { cout << movieVector[i].title << endl; } cout << "Bye world!" << endl; //This life has passed. return 0; } 

Answer 1

The order in which you enter the user and initialize the movies_t object is a red herring. Actual problem here:

 //Create new movie movies_t amovie; movies_t * pmovie; pmovie = &amovie; /* ... */ //Start new thread newThreadStruct startNewThread(movieTitle,movieYear,pmovie,movieVectorPointer); group.create_thread(startNewThread); 

You pass the address of the local variable ( amovie ) to the stream. You do not have direct control when this thread starts, when it tries to access the pointer that you passed to it, or when it exits. But you do not wait in the loop for the main thread of the workflow to use local. As soon as the loop of the loop, the variable that you skip goes beyond. When a worker thread tries to use it, you invoke undefined behavior. This is very bad.

Probably the easiest (and most naive) way to fix this is to dynamically create an amovie object:

 //Create new movie movies_t * pmovie = new movies_t; 

... and then when you finish using it, delete it somewhere. Starting from my initial check of your code, where before delete it was not immediately obvious - but it could be at the end of main .

This naive approach opens up a huge can of worms with respect to pointer ownership, memory management and potentially deadlock and race conditions. Now you have entered the field of multi-threaded programming - one of the most complex things that you need to do correctly is in C ++ programming. The above approach will "work" (as in the case if your code does not crash), albiet is not without flaws, but if you are following the path of multi-threaded programming, it's time to start learning how to do it right. This is a way that goes beyond my short post here.

Answer 2

The error in these lines:

 movies_t amovie; movies_t * pmovie; pmovie = &amovie; 

Here you create a local variable amovie and assign a pointer to a local variable. When a local variable goes out of scope, that is, when the cycle moves or the cycle ends, the memory occupied by the variable is no longer valid. Thus, the pmovie pointer points to unused memory.

You must highlight the pointer using new :

 movies_t *pmovie = new movie_t;