My matrix class crash on initialisation

Question

Issue : program crashing

here is my matrice.h :

class matrice {

public:
    matrice(int nbColumnTMP, int nbLineTMP);

    std::vector<std::vector<int>> returnArray();

    virtual ~matrice();

    void addValue(int numColumn, int numLine, int value);
    void editValue(int numColumn, int numLine, int numValue, int newValue);

private:
    std::vector<std::vector<int>>* array;
    int nbColumn;
    int nbLine;

};

And the .cpp :

matrice::matrice(int nbColumnTMP, int nbLineTMP) {

    nbColumn = nbColumnTMP;
    nbLine = nbLineTMP;

    std::vector<int>* array2;

    for(int C = 0; C < nbColumn; C++)
    {
        array->push_back(*array2);//<- DONT WORK, CRASH

        for(int L = 0; L < nbLine; L++)
        {
            array[C][L].push_back(0); //Init matrix with 0 values
        }
    }
}

std::vector<std::vector<int>> matrice::returnArray()
{
    return *array;
}

matrice::~matrice()
{
    for(int C = 0; C < nbColumn; C++)
    {
        for(int L = 0; L < nbLine; L++)
        {
            for(int V = 0; V < int(array[C][L].size()); V++)
            {
                delete &array[C][L][V];
            }

            delete &array[C][L];
        }

        delete &array[C];
    }

    delete  &nbColumn;
    delete &nbLine;

    delete array;
}

void matrice::addValue(int numColumn, int numLine, int value)
{
    array[numColumn][numLine].push_back(value);
}

void matrice::editValue(int numColumn, int numLine, int numValue, int newValue)
{
    array[numColumn][numLine][numValue] = newValue;
}

After a few tests, i found out that this make my program crash:

array->push_back(*array2);

I'm new to this so i'm not sure i'm doing things well, and i have no clue why this is not working...

I'm using this class for my 2D isometric engine, i want to load some coordinates into them to easily get isometrics coordinates after.

Answer 1

The reason for your issues is that you're using a pointer member, and attempting to write to uninitialized memory.

There is no need for this member to be a pointer:

   std::vector<std::vector<int>>* array;

Instead, make it an object and size it appropriately in the constructor and other functions that size the vector:

   std::vector<std::vector<int>> array;

and then:

matrice::matrice(int nbColumnTMP, int nbLineTMP) : 
             array(nbColumnTMP, std::vector<int>(nbLineTMP))
{}

Also, there is no need to be holding onto variables that denote the number of rows and columns. Use the appropriate vector::size() methods. There is no need for nbColumn and nbLine member variables.

Using extraneous variables can invariably lead to bugs down the road (due to not updating them properly), or other issue that keeps them out-of-sync with the actual size of the vector. For example, your function to add a value -- since you seem to be creating a ragged 2 dimensional vector, those member variables denoting the number of rows and columns become useless, and the only thing reliable is to use the std::vector<T>::size() function.

Also, once you do this, there is no need for a destructor in your matrix class. The vector will clean itself up automatically when the object goes out of scope.

In general, if you want a 2 dimensional or 3 dimensional vector, and you want to start out "easy", then just declare one. Don't go the pointer route. If you did that, then it just becomes a matter of fixing a few syntax errors (or getting used to the syntax), and that is the easy stuff. Going headlong into pointers and dynamically allocated memory will lead you into all sorts of issues that you need not have to get into.

Edit:

For a 3 dimensional vector,

std::vector<std::vector<std::vector<int>>>;

or better yet:

typedef std::vector<int> Int1D;
typedef std::vector<Int1D> Int2D;
typedef std::vector<Int2D> Int3D;
Int3D array;

and then:

matrice::matrice(int nPages, int nbColumnTMP, int nbLineTMP) : 
             array(nPages, Int2D(nbColumnTMP, Int1D(nbLineTMP)))
{}

Answer 2

It looks like you are also new to c++.

As was mentioned in the comment, you first need to initialize the array. Even better would be not to use pointers at all.

in your .h

class matrice {

public:
    matrice(int nbColumnTMP, int nbLineTMP);

    const std::vector<std::vector<std::vector<int>>>& returnArray() const;


    void addValue(int numColumn, int numLine, int value);
    void editValue(int numColumn, int numLine, int numValue, int newValue);

private:
    std::vector<std::vector<std::vector<int>>> array;
};

and .cpp

matrice::matrice(int nbColumnTMP, int nbLineTMP)  {

    array.resize(nbColumnTMP);

    for(int C = 0; C < nbColumnTMP; C++)
    {
        array.push_back(std::vector<std::vector<int>>(nbLineTMP));
    }
}

const std::vector<std::vector<std::vector<int>>>& matrice::returnArray() const
{
    return array;
}

void matrice::addValue(int numColumn, int numLine, int value)
{
    array[numColumn][numLine].push_back(value);
}


void matrice::editValue(int numColumn, int numLine, int numValue, int newValue)
   {
     // you should also add boundary checks for numColumn and numLine
     std::vector<int>& element = array[numColumn][numLine];
     if( numValue >= 0 && numValue < element.size() ){
        element[numValue] = newValue;
     }
   }

There are many excellent matrix packages avaliable, don't implement your own unless they don't support your needs. I use Eigen in the past, it significantly reduced CPU load of our core matrix operations.