C++

Forum

 
Probably a destructor error when running operator= override

DTOK (3)
Hi,
I am new to CPP and I am trying to create a matrix class while using template and some operators overload.

I wrote an operator= override function and when I run it, the debugger opens up a delete_scalar.cpp file and writes that the project "has triggered a breakpoint.
When I run the main without activating the = operator everything works ok.

From trying to change the code and understand what the problem is better I got the error ctrl valid heap pointer(block).

In general I think that the problem is that the destructor is not running correctly for me, and I have no idea why.
Any help will be greatly appricieted.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
 
  The header file (functions inline):

#pragma once
#define DEFAULT_VALUE 0

#include <iostream>	
#include <ostream>	

template <int rows, int cols, typename T = int>
class Matrix
{
public:
	Matrix(T matSetValue = NULL) : m_rows(rows), m_cols(cols)
	{
		m_arr = new T*[m_rows];
		for (int i = 0; i < m_rows; i++)
		{
			m_arr[i] = new T[m_cols];
			for (int j = 0; j < cols; j++)
			{
				if (matSetValue == NULL)
					m_arr[i][j] = 0;
				else
					m_arr[i][j] = matSetValue;
			}	
		}
	};

	~Matrix()
	{
		for (int i = 0; i < this->m_rows; i++)
		{
			delete[] &m_arr[i];
		}
		delete[] &m_arr;
		
	};

	int getNumOfRows() const;
	int getNumOfCols() const;
	T getCellValue(const int& row, const int& col) const;

	void setCellValue(const int& row, const int& col,  int value);

	Matrix<rows, cols, T> operator=(const Matrix<rows, cols, T>& matrix);

private:
	int m_rows;
	int m_cols;
	T** m_arr;

};


template <int rows, int cols, typename T>
inline int Matrix<rows, cols, T>::getNumOfRows() const
{
	return this->m_rows;
};
template <int rows, int cols, typename T>
inline int Matrix<rows, cols, T>::getNumOfCols() const
{
	return this->m_cols;
};
template <int rows, int cols, typename T>
inline T Matrix<rows, cols, T>::getCellValue(const int& row, const int& col) const
{
	return this->m_arr[row][col];
};
template <int rows, int cols, typename T>
inline void Matrix<rows, cols, T>::setCellValue(const int& row, const int& col, int value) 
{
	this->m_arr[row][col] = value;
};

template <int rows, int cols, typename T = int>
std::ostream& operator<<(std::ostream& os, const Matrix<rows, cols, T>& matrix)
{
	int currRows = matrix.getNumOfRows();
	int currCols = matrix.getNumOfCols();
	for (int i = 0; i < currRows; i++)
	{
		for (int j = 0; j < currCols; j++)
		{
			os << " " << matrix.getCellValue(i, j);
		}
		os << std::endl;
	}
	return os;
};

template <int rows, int cols, typename T>
inline Matrix<rows,cols,T> Matrix<rows, cols, T>::operator=(const Matrix<rows, cols, T>& matrix)
{
	int currRows = matrix.getNumOfRows();
	int currCols = matrix.getNumOfCols();
	Matrix<rows, cols> newMat;
	
	for (int i = 0; i < currRows; i++)
	{
		for (int j = 0; j < currCols; j++)
		{
			newMat.m_arr[i][j]=matrix.m_arr[i][j];
		}
	}
	return newMat;
};

Main:
#include "matrix.h"

int main() {

    Matrix<4, 4> mat;
    std::cout << mat << std::endl;

    Matrix<4, 4> identity(1);
    std::cout << identity << std::endl;

    Matrix<4, 4> mat4(3);
    Matrix<4, 4> res = mat4;;
    std::cout << res << std::endl;
Ganado (6786)
You shouldn't be calling delete[] on the address of the pointer. You should be calling delete on the pointer itself.

1
2
3
4
5
 
for (int i = 0; i < this->m_rows; i++)
{
	delete[] m_arr[i];
}
delete[] m_arr;


1
2
3
4
 
if (matSetValue == NULL)
	m_arr[i][j] = 0;
else
	m_arr[i][j] = matSetValue;
This is quite redundant, and is equivalent to:
m_arr[i][j] = matSetValue;

NULL is meant for pointers; it may have the value zero when converted into an int, but semantically it will confuse readers if you use it like this. Just use 0. Or, you can initialize the variable in a generic way by doing
T value = T();
Last edited on
DTOK (3)
Thanks for the if- else correct.

Removing the & signs to delete the pointer itself shows another problem now:
Exception thrown at 0x7920DB1B (ucrtbased.dll) in Try.exe: 0xC0000005: Access violation reading location 0xDDDDDDCD.

Will love the help again since I couldn't find anything to fix this as well.
Ganado (6786)
You need a copy constructor.
https://www.tutorialspoint.com/cplusplus/cpp_copy_constructor.htm

But also, don't return a copy of your matrix from your operator=.
Just return a reference.
So change return type from Matrix<rows,cols,T> to Matrix<rows,cols,T>&
Last edited on
seeplus (6475)
There is big problem with operator=. Consider:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
 
#include <iostream>
#include <ostream>
#include <utility>
#include <algorithm>

template <size_t rows, size_t cols, typename T = int>
class Matrix {
public:
	Matrix(T matSetValue = T {}) : m_rows(rows), m_cols(cols), m_arr(new T* [m_rows]) {
		for (size_t i = 0; i < m_rows; ++i) {
			m_arr[i] = new T[m_cols];
			std::fill_n(m_arr[i], m_cols, matSetValue);
		}
	}

	~Matrix() {
		for (size_t i = 0; i < m_rows; ++i)
			delete[] m_arr[i];

		delete[] m_arr;
	}

	Matrix(const Matrix<rows, cols, T>& mat);

	inline size_t getNumOfRows() const;
	inline size_t getNumOfCols() const;

	inline T getCellValue(const size_t& row, const size_t& col) const;
	inline void setCellValue(const size_t& row, const size_t& col, int value);

	Matrix<rows, cols, T>& operator=(const Matrix<rows, cols, T> matrix);

	Matrix<rows, cols, T>& swap(Matrix<rows, cols, T>& mat) {
		std::swap(m_arr, mat.m_arr);
		std::swap(m_rows, mat.m_rows);
		std::swap(m_cols, mat.m_cols);

		return *this;
	}

private:
	size_t m_rows {};
	size_t m_cols {};
	T** m_arr {};
};


template <size_t rows, size_t cols, typename T>
inline size_t Matrix<rows, cols, T>::getNumOfRows() const
{
	return m_rows;
};

template <size_t rows, size_t cols, typename T>
inline size_t Matrix<rows, cols, T>::getNumOfCols() const
{
	return m_cols;
};

template <size_t rows, size_t cols, typename T>
inline T Matrix<rows, cols, T>::getCellValue(const size_t& row, const size_t& col) const
{
	return m_arr[row][col];
};

template <size_t rows, size_t cols, typename T>
inline void Matrix<rows, cols, T>::setCellValue(const size_t& row, const size_t& col, int value)
{
	m_arr[row][col] = value;
};

template <size_t rows, size_t cols, typename T = int>
std::ostream& operator<<(std::ostream& os, const Matrix<rows, cols, T>& matrix)
{
	const auto currRows {matrix.getNumOfRows()};
	const auto currCols {matrix.getNumOfCols()};

	for (size_t i = 0; i < currRows; ++i) {
		for (size_t j = 0; j < currCols; ++j)
			os << ' ' << matrix.getCellValue(i, j);

		os << '\n';
	}

	return os;
};

template <size_t rows, size_t cols, typename T>
inline Matrix<rows, cols, T>::Matrix(const Matrix<rows, cols, T>& matrix) : m_rows(matrix.m_rows), m_cols(matrix.m_cols), m_arr(new T* [matrix.m_rows]) {
	for (size_t i = 0; i < m_rows; ++i) {
		m_arr[i] = new T[m_cols];
		std::copy_n(matrix.m_arr[i], m_cols, m_arr[i]);
	}
}

template <size_t rows, size_t cols, typename T>
inline Matrix<rows, cols, T>& Matrix<rows, cols, T>::operator=(const Matrix<rows, cols, T> matrix)
{
	swap(*this, matrix);

	return *this;
};

int main()
{
	Matrix<4, 4> mat;
	std::cout << mat << '\n';

	Matrix<4, 4> identity(1);
	std::cout << identity << '\n';

	Matrix<4, 4> mat4(3);
	Matrix<4, 4> res {mat4};
	std::cout << res << '\n';
}

Last edited on
Ganado (6786)
Of course, not sure how I missed that. DTOK, to clarify, the issue I didn't see is that you are creating and modifying a local instance of the Matrix inside your operator=, and not the actual object that the operator= is being called on.
Last edited on
DTOK (3)
Adding a copy constructor, returning a reference helped.

Here is my code if it helps anybody in the future:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
 
#pragma once
#define DEFAULT_VALUE 0

#include <iostream>	
#include <ostream>	

template <int rows, int cols, typename T = int>
class Matrix
{
public:
	Matrix(T matSetValue = NULL) : m_rows(rows), m_cols(cols)
	{
		m_arr = new T*[m_rows];
		for (int i = 0; i < m_rows; i++)
		{
			m_arr[i] = new T[m_cols];
			for (int j = 0; j < cols; j++)
			{
				if (matSetValue == NULL)
					m_arr[i][j] = 0;
				else
					m_arr[i][j] = matSetValue;
			}	
		}
	};

Matrix(const Matrix<rows, cols, T>& mat) 
	{
		*this = mat;
	};

	~Matrix()
	{
		for (int i = 0; i < this->m_rows; i++)
		{
			delete[] &m_arr[i];
		}
		delete[] &m_arr;
		
	};

	int getNumOfRows() const;
	int getNumOfCols() const;
	T getCellValue(const int& row, const int& col) const;

	void setCellValue(const int& row, const int& col,  int value);
void setMatValue(int value);

	Matrix<rows, cols, T>& operator=(const Matrix<rows, cols, T>& matrix);

private:
	int m_rows;
	int m_cols;
	T** m_arr;

};


template <int rows, int cols, typename T>
inline int Matrix<rows, cols, T>::getNumOfRows() const
{
	return this->m_rows;
};
template <int rows, int cols, typename T>
inline int Matrix<rows, cols, T>::getNumOfCols() const
{
	return this->m_cols;
};
template <int rows, int cols, typename T>
inline T Matrix<rows, cols, T>::getCellValue(const int& row, const int& col) const
{
	return this->m_arr[row][col];
};
template <int rows, int cols, typename T>
inline void Matrix<rows, cols, T>::setCellValue(const int& row, const int& col, int value) 
{
	this->m_arr[row][col] = value;
};

template <int rows, int cols, typename T = int>
std::ostream& operator<<(std::ostream& os, const Matrix<rows, cols, T>& matrix)
{
	int currRows = matrix.getNumOfRows();
	int currCols = matrix.getNumOfCols();
	for (int i = 0; i < currRows; i++)
	{
		for (int j = 0; j < currCols; j++)
		{
			os << " " << matrix.getCellValue(i, j);
		}
		os << std::endl;
	}
	return os;
};

template <int rows, int cols, typename T>
inline Matrix<rows,cols,T>& Matrix<rows, cols, T>::operator=(const Matrix<rows, cols, T>& mat)
{
	this->m_rows =mat.getNumOfRows();
	this->m_cols = mat.getNumOfCols();
	this->m_arr = new T * [mat.getNumOfRows()];
	for (int i = 0; i < m_rows; i++)
	{
		m_arr[i] = new T[m_cols];
		for (int j = 0; j < m_cols; j++)
		{
			this->m_arr[i][j] = mat.m_arr[i][j];
		}
	}
	return *this;
};

Main:
#include "matrix.h"

int main() {

    Matrix<4, 4> mat;
    std::cout << mat << std::endl;

    Matrix<4, 4> identity(1);
    std::cout << identity << std::endl;

    Matrix<4, 4> test(16);
    Matrix<4, 4> res = test;

	std::cout << "the test values are: " << std::endl;
	std::cout << test << std::endl;
	std::cout << "the res values are: " << std::endl;
	std::cout << res << std::endl;

	test.setMatValue(1993);
	std::cout << "the test values are: " << std::endl;
	std::cout << test << std::endl;
	std::cout << "the res values are: " << std::endl;
	std::cout << res << std::endl;
Last edited on
seeplus (6475)
No. Your operator=() still has issues. If you do it this way - as opposed to the way in my previous post - then you have to check that you are not assigning to yourself and if not then you need to free the existing memory before allocating new memory. As coded, there is a major memory leak.

You are implementing copy constructor in terms of operator=. Whilst this isn't wrong per sec, it's more usual to implement operator= in terms of copy constructor/destructor.

L19-21 aren't needed as mentioned above. Also the default for matSetValue should be the default for that type.

Within a class, you don't need to use this-> to access a class member. The default is to use the class member.
Last edited on
Topic archived. No new replies allowed.