Private members of class changes during instantiation of my class even though they shouldn't

Question

My question is: Why do the my functions GetNumberOfRows() and GetNumberOfColumns() not return the values I specified in my constructor?

I have a class template that allocates memory for Matrix stored as a std::vector. As private members of this class I have the number of rows and columns (mNumRows, mNumCols). I have a constructor that takes number of rows and columns as arguments and creates a std::vector to store the values in. I have created assertions to make sure that when I index, the matrix, the index must not exceed the number of rows or columns. When overloading the *-operator I assert that matrix columns and vector size are identical (when multiplying my matrix class with a vector class). My program compiles, but during execution of my program I kept getting assertion errors, and I therefore checked the values of mNumRows and mNumCols, which proved to be 31, and 0, even though I specified them to be 2 and 2 in the constructor.

My program gives the correct results when specifying mNumRows and mNumCols to be a number inside the class, but I want to be able to specify it in my main file. I have provided my header file, where my class is specified.

template<typename T> class Matrix
{
private:
   std::vector<T> mData; // entries of matrix
   int mNumRows;
   int mNumCols; // dimensions
   int N;

public:
    // copy constructor
   Matrix(const Matrix& otherMatrix)
   {
    mNumRows = otherMatrix.mNumRows;
    mNumCols = otherMatrix.mNumCols;
    std::vector<T> mData;
    for (int i = 0; i < otherMatrix.N; i++)
    {
    mData = otherMatrix.mData;
    }
   }

   // Storing matrix as a flattened matrix
   // And using std::vector
   Matrix(int numRows, int numCols)
   {
    assert(numRows > 0);
    assert(numCols > 0);
    int mNumRows = numRows;
    int mNumCols = numCols;
    int N = mNumRows * mNumCols;
    for (int i = 0; i < N; i++)
    {
    mData.push_back(0.0);
    }
   }

   ~Matrix()
   {
      mData.clear();
   }

   int GetNumberOfRows() const
   {
       return mNumRows;
   }

   int GetNumberOfColumns() const
   {
       return mNumCols;
   }
};

In the main file I write.

Matrix mat(2, 2);

std::cout << mat.GetNumberOfRows() << ", " << mat.GetNumberOfColumns() << std::endl;

mat.~Matrix();

The output is 31, 0. It should be 2, 2. Why does the rows and columns change?

Just some minor issues in advance: `assert(numRows > 0);` – you don't need that, if you accept `unsigned int` as parameter. Maybe, though (no matter if signed or unsigned) there's some reasonable upper limit? `N` is redundant, you get the same via `mData.size()`. `mData = otherMatrix.mData;` already copies the entire vector, if you do it in a loop, you do N times exactly the same task again and again. Actually, as you have no user defined memory management, the default copy constructor is fine (`Matrix(Matrix const& otherMatrix) = default;` – yours, in contrast, isn't...). — Aconcagua, Jul 31 '19 at 10:39
Get used to using the constructor's initialiser list (not to be confused with `std::initializer_list`!): `Matrix(unsigned int numRows, unsigned int numCols) : m(numRows), mNumCols(numCols), mData(numRows*numCols, 0.0) { }` – you avoid default initialisation + assignment in favour of direct initialisation by value; some types (references and non-default-constructable ones) *only* can be initialised that way. Additionally: `std::vector` has a constructor that already does what you do in the loop (just more efficiently, especially, as you do not `reserve` in advance), I used it already above... — Aconcagua, Jul 31 '19 at 10:45
Really, really bad: `Matrix mat(2, 2); mat.~Matrix();` will result in double deletion of your object, as destructor is called implicitly (*again* in this case) as soon as `mat` runs out of scope. There's almost never need for calling a destructor explicitly (exception is if you created your objects with placement-new). — Aconcagua, Jul 31 '19 at 10:54
It seems like you could use [a few good books to read](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list/388282#388282). — Some programmer dude, Jul 31 '19 at 10:55
You don't need to `clear` a `std::vector` in your custom destructor, `std::vector` will clean up everything properly in its own destructor (so at best you achieve nothing, at worst produce extra work). — Aconcagua, Jul 31 '19 at 10:56
Correction of one of my previous comments (haven't paid attention myself!): If using constructors initialiser list, you need to initialise the members in the order they are declared inside the class (which I failed to comply with), needs to be: `Matrix(...) : mData(...), mNumRows(...), mNumCols(...) { }` – sorry for confusion. — Aconcagua, Jul 31 '19 at 11:04

Some programmer dude · Accepted Answer · 2019-07-31T11:05:23.957

The problem is because of these three lines in the constructor:

int mNumRows = numRows;
int mNumCols = numCols;  // Corrected from question which had assignment from numRows
int N = mNumRows * mNumCols;

They define three new and local variables, which have no relation to the member variables of the same name. All changes to these local variables are lost when the constructor function ends.

One solution is to use plain assignment for the member variables:

mNumRows = numRows;
mNumCols = numCols;
N = mNumRows * mNumCols;

But I rather recommend you learn about constructor initializer lists where you can directly initialize member variables:

Matrix(int numRows, int numCols)
    : mNumRows(numRows), mNumCols(numCols), N(mNumRows * mNumCols)
{
    // The rest of the constructor...
}

You have the same problem in the copy-constructor where you define a new local variable mData which shadows the member variable of the same name.

With a little knowledge of the std::vector constructors you don't need anything in the constructor function at all, it can all be initialized using the constructor initializer list:

Matrix(int numRows, int numCols)
    : mData(numRows * numCols), mNumRows(numRows), mNumCols(numCols)
{
    // Totally empty!
}

Do note that I don't initialize N. That's because it's not needed, you can get the value of N from the vector through its size function.

Also note that I don't specify a default value for the contents of the vector. That's because the values will be default-constructed. And for simple types like int or float that means the values will be zero.

That error was in question already, failed to edit my comment appropriately in time... — Aconcagua, Jul 31 '19 at 10:48
But please add a line of comment that you fixed that as well, as it was already an error in the question. — Aconcagua, Jul 31 '19 at 10:51

Vlad from Moscow · Answer 2 · 2019-08-01T14:07:14.960

The class definition entirely does not make sense.

For example consider the following copy constructor.

   Matrix(const Matrix& otherMatrix)
   {
    mNumRows = otherMatrix.mNumRows;
    mNumCols = otherMatrix.mNumCols;
    std::vector<T> mData;
    for (int i = 0; i < otherMatrix.N; i++)
    {
    mData = otherMatrix.mData;
    }
   }

There is declared a local cector

std::vector<T> mData;

that is named as the corresponding data member of the class.

Then in the loop

for (int i = 0; i < otherMatrix.N; i++)
{
mData = otherMatrix.mData;
}

the local vector N times is assigned by the vector otherMatrix.mData.

In this constructor

   Matrix(int numRows, int numCols)
   {
    assert(numRows > 0);
    assert(numCols > 0);
    int mNumRows = numRows;
    int mNumCols = numRows;
    int N = mNumRows * mNumCols;
    for (int i = 0; i < N; i++)
    {
    mData.push_back(0.0);
    }
   }

there are also used local variables instead of the corresponding data members of the class

int mNumRows = numRows;
int mNumCols = numRows;

Moreover there is a typo

int mNumCols = numRows;
               ^^^^^^^

This constructor could look for example like

Matrix(int numRows, int numCols) 
   : mData( numRows * numCols ), mNumRows( numRows ), mNumCols( numCols ), N( numRows * numCols )
{
    assert(numRows > 0);
    assert(numCols > 0);
}

Also it is a bad idea to call explicitly the destructor of a variable with automatic storage duration

mat.~Matrix();

This can result in undefined behavior.

Here is a demonstrative program that shows how your code can be rewritten

#include <iostream>
#include <vector>

template<typename T = int> 
class Matrix
{
private:
    size_t mNumRows;
    size_t mNumCols; // dimensions
    size_t N;
    std::vector<T> mData; // entries of matrix

public:
    // copy constructor
    Matrix( const Matrix &otherMatrix ) 
        : mNumRows( otherMatrix.mNumRows ),
          mNumCols( otherMatrix.mNumCols ),
          N( otherMatrix.mNumRows * otherMatrix.mNumCols ),
          mData( otherMatrix.mData )
    {
    }

   // Storing matrix as a flattened matrix
   // And using std::vector
    Matrix( size_t numRows, size_t numCols ) 
        : mNumRows( numRows ),
          mNumCols( numCols ),
          N( numRows * numCols ),
          mData( N )
    {
    }

    ~Matrix() = default;

    size_t GetNumberOfRows() const
    {
        return mNumRows;
    }

    size_t GetNumberOfColumns() const
    {
        return mNumCols;
    }
};

int main()
{
    Matrix<> mat(2, 2);

    std::cout << mat.GetNumberOfRows() << ", " << mat.GetNumberOfColumns() << std::endl;
}

The program output is

2, 2

Private members of class changes during instantiation of my class even though they shouldn't

2 Answers2