c++pointersvectorforward-list

Vector data is lost after it goes out of scope


I'm working on a project where I use a forward list and vector together and output their values. The forward list is a list of Frame objects, and the vector is a vector of Display objects. The issue is that after the creation of the objects in InsertFrame(), the values of the vector are lost and essentially become garbage. I can see in the debugger it happens right after the function ends, which leads me to believe it has to do with the variables going out of scope. Here is the main creation class

// Animation.cpp


#include <crtdbg.h>
#include <iostream>
#include <string>
#include <vector>
#include <forward_list>
using namespace std;

#include "Display.h"
#include "Frame.h"
#include "Animation.h"
#include "GPUMemoryDisplay.h"
#include "SystemMemoryDisplay.h"


    void Animation::InsertFrame() {

        int numDisplays;        //for user input of display number
        vector <Display*>v;     //vector for containing display objects
        int p_x;                //will contain user input for pixel_x
        int p_y;                //will contain user input for pixel_y
        int p_duration;         //will contain user input for duration
        int p_type ;            //will contain display type as int value
        char * p_name;          //temp string to contain user input for name
        string d_name;          //will contain p_name to be passed to display constructor
        string frameName;       //contains user input for the frame name
        string gpu_shader;      //contains gpu name if gpu type is selected
        int q = 0;              //used to count the diplay #

        //begin reading user input
        cout << "Insert a Frame in the Animation\nPlease enter the Frame filename: ";
        cin >> frameName;
        cout << "Entering the Frame Displays (the sets of dimensions and durations) " << endl;
        cout << "Please enter the number of Displays: ";
        cin >> numDisplays;

        //display creation loop for # of displays entered
        while (numDisplays > 0) {
            cout << "Please enter pixel x-width for Display #" << q << " pixel_x:";
            cin >> p_x;
            cout << "Please enter pixel y-width for Display #" << q << " pixel_y:";
            cin >> p_y;
            cout << "Please enter the duration for this Display: ";
            cin >> p_duration;
            cout << "Please enter the name for this Display: ";
            cin >> d_name;
            cout << "Please enter the type for this display (1 = SystemMemoryDisplay, 2 = GPUMemoryDisplay): ";
            cin >> p_type;
            p_name = new char[d_name.length() + 1]; //allocate for the size of the name entered
            strcpy(p_name, d_name.c_str()); //copy string to char []

            if (p_type == 2) {
                //input for GPU shader
                cout << "Please enter the file name of the associated GPU Shader: ";
                cin >> gpu_shader;
                Display *gpu_p = new GPUMemoryDisplay(p_x, p_y, p_duration, p_name, gpu_shader);
                v.push_back(static_cast <Display*>(gpu_p)); //casting to a display* and pushing onto the vector
                numDisplays--;
                q++;
            }
            else {
                Display *sm_p = new SystemMemoryDisplay(p_x, p_y, p_duration, p_name);
                v.push_back(static_cast <Display*>(sm_p));//casting to a display* and pushing onto the vector
                numDisplays--;
                q++;
            }
            cout << "\n";
        }


        Frame t_frame = Frame(frameName, v); //new frame holds vector which contains displays


        //check if forward list is empty
        if (frames.empty()) {
            cout << "\nThis is the first Frame in the list \n\n";
            frames.push_front(t_frame);
        }
        else {
            forward_list <Frame>::iterator it;
            int x = 0; // used for size of current forward_list
            //iterate forward list to obtain the size
            for (it = frames.begin(); it != frames.end(); ++it) {
                x++;
            }
            if (x == 1) {
                it = frames.begin();
                frames.insert_after(it, t_frame);
            }
            else {

                cout << "There are " << x << " Frame(s) in the list\n" << "Please specify the position, between 0 and " << x << " to insert after : ";
                cin >> x; //read in where user wants to put the frame

                //iterate to desired position and insert
                forward_list <Frame>::iterator it;
                it = frames.begin();
                while (x != 0 && it != frames.end()) {
                    it++;
                    x--;
                }
                frames.insert_after(it, t_frame);
            }
        }
    }

And the header/cpp files for Frame and Display

// Frame.h
#pragma once

    class Frame
    {
        string fileName;
        vector<Display*> displays; 
    public:
        Frame(string s, vector<Display*> d) :fileName(s), displays(d) {}
        Frame(const Frame&);
        ~Frame()
        {
            vector<Display*>::iterator it;
            for (it = displays.begin(); it != displays.end(); it++)
                delete *it;
        }
        friend ostream& operator<<(ostream&, Frame&);
    };

 #pragma once
// Display.h

    class Display
    {
    protected:  // accessible to derived classes
        int pixel_x;
        int pixel_y;
        int duration;
        char* name;
    public:
        Display(int x, int y, int duration, char* name);
        Display(const Display&);
        virtual ~Display() //makes class abstract, cannot be instantiated, most general class
        {
            if (name)
                delete[]name;
        }
        virtual int BufferSize() = 0; // overridden function Polymorphic function
        friend ostream& operator<<(ostream&, Display&);
    };

// Display.cpp

#include <crtdbg.h>
#include <iostream>
#include <string>
#include <vector>
#include <forward_list>
using namespace std;
#include "Display.h"
#include "GPUMemoryDisplay.h"

Display::Display(int x, int y, int d, char* n) :pixel_x(x), pixel_y(y), duration(d), name(n) {
}


Display::Display(const Display& p) {
    //copy values from p
    pixel_x = p.pixel_x;
    pixel_y = p.pixel_y;
    duration = p.duration;
    size_t len = strlen(p.name);
    name = new char[len + 1];
    strcpy(name, p.name);
    //cout << pixel_x << pixel_y << duration << name;

}

I have two sub classes of Display called GPUMemoryDisplay and SystemMemoryDisplay, however I believe that part of the code is fine as I can see their values stored correctly in the debugger. Included below just in case.

#pragma once
// SystemMemoryDisplay.h

class  SystemMemoryDisplay : public Display
{
public:
    SystemMemoryDisplay(int x, int y, int duration, char* name) :Display(x, y, duration, name) {};
    SystemMemoryDisplay(const SystemMemoryDisplay& RGMD) :Display(RGMD) {}
    int BufferSize() { return pixel_x*pixel_y * sizeof(double); }
}; 


#pragma once
//GPUMemoryDisplay.h
//this is the derived class of display
class GPUMemoryDisplay : public Display
{
    string shader;
public:
    GPUMemoryDisplay(int x, int y, int duration, char* name, string shader) :Display(x, y, duration, name), shader(shader) {};
    GPUMemoryDisplay(const GPUMemoryDisplay& RGPUMD) :shader(RGPUMD.shader), Display(RGPUMD) {}
    string GetShader() { return shader; }
    int BufferSize() { return pixel_x*pixel_y * sizeof(float); } //this is the overridden function from Display class
};

In summary I have a forward list of frames, each frame can contain a vector of Display objects. However when the InsertFrame() function exits, the display data is lost.


Solution

  • Once your stack-allocated Frame t_frame = Frame(frameName, v); objects go out of scope their destructors are called and will delete all the objects pointed by pointers stored in Frame::displays. You need to implement appropriate copy and / or move constructor and assignment operators that will transfer those pointers correctly. You should use ::std::unique_ptr to keep ownership of allocated objects instead of using raw pointers and deleting them manually and ::std::string to mange strings instead of raw pointer to char. Also putting using namespace std; in-between includes is also no good, if you are going to use it at least place it after includes.