Assertion error when passing object by value -- it is my copy constructor?

Question

everyone!

I just finished writing a 2-D maze (Class is an ADT titled "Maze"--how original) that uses dynamic memory allocation. I'm passing the Maze to a method of another class I've entitled "MazeSolver," which uses recursion and backtracking to solve the maze. Good news is my code compiles wonderfully when I pass the object by reference. News that I don't know if is good or bad is that I get an assertion error if I try to pass the Maze to MazeSolver by value.

Given that the error occurs only when I pass by value, I can only assume it has something to do with my copy constructor. Before going any further, here's some info on the code:

Maze is composed of squares. Each square is represented by a struct called SquareData.

      struct SquareData 
    {
        //data fields for the struct (NO POINTERS)
    }

I've decided to represent the entire maze with a vector of SquareData pointers (this vector is in private section of the class "Maze").

vector<SquareData*> squares;

Implementation of my destructor looks like this (that last call referencing a Player class is just eliminating a dangling pointer I have declared as a static variable for that class, which I have pointing at the maze. I don't think it's important considering the question, but I am new to C++ after all and one of you may think it might be, so I've included it for "hmmms"):

// clears maze of its contents
void Maze::clear() {
    int totalSquares = squares.size();
    for (int loopVar = 0; loopVar < totalSquares; loopVar++)
    {
        delete squares[loopVar]; // deallocate memory by deleting the square structure
        squares[loopVar] = nullptr; // eliminate dangling pointer
    } // vector takes care of itself
} // end clear

Maze::~Maze(){
    //clear the maze of contents (vector is full of pointers whose memory is on the heap)
    clear();
    //vector safe to deallocate itself now
    Player::setMaze(nullptr); // remove the pointer from player
}

I've declared the copy constructor in header as follows:

/** Copy Constructor */
    Maze(const Maze& myMaze);

with attempted implementation:

/** copy constructor */
Maze::Maze(const Maze& myMaze){
    /** Initialize Constants */
    mazeLength = myMaze.mazeLength;
    mazeWidth = myMaze.mazeWidth;
    exitRow = myMaze.exitRow;
    exitCol = myMaze.exitCol;
    entRow = myMaze.entRow;
    entCol = myMaze.entCol;
    /** copy the vector of pointers*/
    for (int loopVar = 0; loopVar < myMaze.squares.size(); loopVar++)
    {
        squares.push_back(myMaze.squares[loopVar]);
    }
} // end copy constructor

Here's how I attempted to understand what the problem was doing:

I wrote this vector display function in for my Maze class.

void Maze::vectorDisplay() const {
    for (int loopVar = 0; loopVar < squares.size(); loopVar++)
    {
        cout << "Vector Index: " << loopVar << endl;
        cout << "Pointer: " << squares[loopVar] << endl;
        cout << "Row: " << squares[loopVar]->Row << endl;
        cout << "Col: " << squares[loopVar]->Col << endl;
        cout << "State: " << squares[loopVar]->State << endl;
    }
} //end vectorDisplay

And found that the vector displays correctly when doing the following in the driver:

    Maze myMazeObject(// parameters);
    myMazeObject.vectorDisplay();

and will produce output with no complaints.

But now if I try to use code like this when passing by value:

Maze myMazeObject(// parameters);
MazeSolver myMazeSolver;
myMazeSolver.someMazeSolverMethod(myMazeObject); 

where someMazeSolverMethod has the line myMazeObject.vectorDisplay();I get an assertion error just as the final element in the vector is being printed.

I want to say this is my fault and my copy constructor is a p.o.s. If any insight, please let me know how to fix it and what I can do in the future!

Thanks for taking the time to read and even more so to answer should you choose to!

-J

Answer 1

Use of

    squares.push_back(myMaze.squares[loopVar]);

in the copy constructor will lead to problems downstream.That will be vaild had the contents of squares been objects not pointers.

There are now two objects holding on to the pointers. Both willl try to call delete on the same pointer, which easily leads to undefined behavior.

You can fix the problem by:

1. Using a vector objects instead of a vector of pointers, or

2. Creating new objects from the heap and adding them to the new object.

   squares.push_back(new SquareData(*myMaze.squares[loopVar]));