Why does returning a data structure rather than a pointer mess with the integrity of my data?

Question

I'm building a sparse matrix class that holds two arrays (row and column) of pointers to doubly linked lists (down and right). Sort of like this:

 rows
c0123456789
o1
l2
u3
m4  A-->B-->
n5  |   |
s6  |   V
 7  V   D-->
 8  C-->
 9  

Both arrays are initialized to have nullptr in every space until something is inserted in that place.

I have a function "readFile" that reads in objects from a text file and inserts them into this sparse matrix. For some reason, before this function returns, all of the data in it is fine, but after I return, I get random memory locations in my arrays. Here is main.cpp

#include <iostream>
#include <string>
#include <fstream>
#include "sparseMatrix.h"

using namespace std;

class basic 
{
private:
    int x, y;
    string word;
    basic *down;
    basic *right;
public:
    basic(int x, int y, string word)
    {
        this->x = x;
        this->y = y;
        this->word = word;
        down = nullptr;
        right = nullptr;
    }
    int getX()
    {
        return x;
    }
    int getY()
    {
        return y;
    }
    basic *getRight()
    {
        return right;
    }
    void setRight(basic *newRight)
    {
        right = newRight;
    }
    basic *getDown()
    {
        return down;
    }
    void setDown(basic *newDown)
    {
        down = newDown;
    }
    void print()
    {
        cout << "X: " << x << ", Y: " << y << ", word: " << word << ".\n";
    }
};

sparseMatrix<basic> readFileBROKEN(string pathToFile);
sparseMatrix<basic> *readFile(string pathToFile);

int main()
{
    cout << "Working:\n\n";
    sparseMatrix<basic> *workingMatrix = readFile("C:/users/jmhjr/desktop/testdata.txt");
    cout << "After returning, here are all the locations that are NOT nullptr:\n";
    workingMatrix->printyArray();
    cin.get();

    cout << "Not working:\n\n";
    sparseMatrix<basic> brokenMatrix = readFileBROKEN("C:/users/jmhjr/desktop/testdata.txt");
    cout << "After returning, here are all the locations that are NOT nullptr:\n";
    brokenMatrix.printyArray();
    cin.get();

    delete workingMatrix;

}

sparseMatrix<basic> readFileBROKEN(string pathToFile)
{
    ifstream inputFile;
    inputFile.open(pathToFile);
    if (inputFile.fail())
    {
        cout << "Couldn't open " << pathToFile << "!\n";
        exit(-1);
    }

    sparseMatrix<basic> matrix(100, 100);

    while (!inputFile.eof())
    {
        int x, y;
        string word;
        inputFile >> x >> y >> word;
        basic data(x, y, word);
        matrix.insert(data);
    }
    cout << "Before returning, here are all the locations that are NOT nullptr:\n";
    matrix.printyArray();
    cout << "press ENTER to return\n";
    cin.get();
    return matrix;
}

sparseMatrix<basic> *readFile(string pathToFile)
{
    ifstream inputFile;
    inputFile.open(pathToFile);
    if (inputFile.fail())
    {
        cout << "Couldn't open " << pathToFile << "!\n";
        exit(-1);
    }

    sparseMatrix<basic> *matrix = new sparseMatrix<basic>(100, 100);

    while (!inputFile.eof())
    {
        int x, y;
        string word;
        inputFile >> x >> y >> word;
        basic data(x, y, word);
        matrix->insert(data);
    }
    cout << "Before returning, here are all the locations that are NOT nullptr:\n";
    matrix->printyArray();
    cout << "press ENTER to return\n";
    cin.get();
    return matrix;
}

and here is sparseMatrix.h:

template <class dataType>
class sparseMatrix
{
private:
        //The dimensions of the sparse matrix.
    int width;
    int height;
        //Dynamic array of pointers to heads of linked lists.
    dataType** xArray;
    dataType** yArray;

public:
        //Constructor. Sets everything in the two arrays to nullptr.
    sparseMatrix(int height, int width)
    {
        this->width = width;
        this->height = height;
        xArray = new dataType*[width];
        yArray = new dataType*[height];
        for (int row = 0; row < height; row++)
        {
            this->yArray[row] = nullptr;
        }
        for (int col = 0; col < width; col++)
        {
            this->xArray[col] = nullptr;
        }
    }

        //Deconstructor. First goes through the matrix and looks for every city it can find, and deletes
        //all of those. Then when it's done, it deletes the two dynamic arrays.
    ~sparseMatrix()
    {
        dataType *currentdataType;
        dataType *next;
        for (int row = 0; row < height; row++)
        {
            currentdataType = yArray[row];
            while (currentdataType != nullptr)
            {
                next = currentdataType->getRight();
                delete currentdataType;
                currentdataType = next;
            }
        }
        delete [] yArray;
        delete [] xArray;
    }


        //Creates a copy of the data we are passed, then creates links to this copy.
    void insert(dataType data)
    {
            //Make sure the data is valid.
        if (data.getX() < 0 || data.getX() >= width || data.getY() < 0 || data.getY() >= height)
        {
            std::cout << "That dataType doesn't fit into the sparse matrix!\n";
            data.print();
            std::cin.get();
        }

        else
        {
                //Copy the data we were passed.
            dataType *newData = new dataType(data);

                //Easy case. If nothing is in this row, set yArray[row] to the address of this data.
            if (yArray[data.getY()] == nullptr)
            {
                yArray[data.getY()] = newData;
            }

                //Not so easy case. Move forward (right) until we find the right location, then set links.
            else
            {
                dataType *current = yArray[data.getY()];
                while (current->getRight() != nullptr)
                {
                    current = current->getRight();
                }
                current->setRight(newData);
            }

                //Easy case. If nothing is in this col, set xArray[col] to the address of this data.
            if (xArray[data.getX()] == nullptr)
            {
                xArray[data.getX()] = newData;
            }

                //Not so easy case. Move forward (down) until we find the right location, then set links.
            else
            {
                dataType *current = xArray[data.getX()];
                while (current->getDown() != nullptr)
                {
                    current = current->getDown();
                }
                current->setDown(newData);
            }
        }
    }
    void printyArray()
    {
        for (int r = 0; r < height; r++)
        {
            if (yArray[r] != nullptr)
            {
                std::cout << r << ' ';
                //yArray[r]->print();
            }
        }
    }
};

readFile reads everything in from a file that looks like this:

0   0   hello
5   2   world
6   8   foo
9   5   bar
...

As expected, before returning, the only locations that are NOT nullptr are the ones that I have inserted into. (0, 2, 8 and 5). However when the function returns, EVERY SINGLE location in the array is not nullptr. I added a second function which returns a pointer to dynamically allocated sparseMatrix object, rather then returning the object itself, and this fixed it. However, I don't understand why. It seems like these two functions should behave identically the same way.

Also, the part that is most confusing to me, why does this run perfectly fine in Xcode, but not in Visual Studio?

Answer 1

tomse's answer is correct and gives the why and a fix, but it's an unnecessarily expensive fix for this problem. His suggestion of the copy constructor also solves numerous future problems such as the classics Why did my vector eat my data? and Dude, where's my segfault? Make the copy constructor. Don't use it unless you have to.

I think Andras Fekete got the problem right, but his post is kind of garbled. His solution is bang on, though.

Define your function like this:

bool readFile(string pathToFile, sparseMatrix<basic> & matrix)

Remove the definition of matrix inside the function in favour of the one passed in.

Return false on error so you know the matrix is bad (or use exceptions).

Create the matrix in the calling function and pass it into the revised reader function.

sparseMatrix<basic> matrix(100, 100);
if readFile("C:/users/jmhjr/desktop/testdata.txt", matrix);

That puts you right back where you were with the pointer version, but without the pointer and without having to do the extra work of copying data you didn't need to copy.

Answer 2

Your function:

sparseMatrix<basic> readFileBROKEN(string pathToFile)

returns a copy of the object (which is OK), but sparseMatrix does not define a copy constructor, so the default generated will be used which creates a shallow copy by just copying the adresses inside the returned object. But the memory where the address points to is deleted when you leave your function (because the destructor of the locally created object is called).

To solve this you have to define your own copy contructor in sparseMatrix which copies all the content of the object.

sparseMatrix(const sparseMatrix& rhs) :
  width(rhs.width),
  height(rhs.height),
  xArray(nullptr),
  yArray(nullptr)
{
  ... and now copy all the content from rhs.xArray to this->xArray,
  (same for yArray)
}

Answer 3

The problem is that you're allocating 'matrix' inside both of the readFile functions. Upon returning from the function, both variables are deallocated. However, returning the value (eradFile) the matrix is copied into your variable of the calling function, whereas returning the pointer (readFileBROKEN) is just returning the address where the matrix used to be stored.

To fix this, you should allocate the 'matrix' variable, and pass in a reference to the function. Then the function can return a void while stuffing the matrix properly.