So I am trying to overload two operators for my "Matrix" class (+ and +=). I am attempting to make +
chainable and +=
non-chainable:
template <class T>
Matrix<T>& Matrix<T>::operator+=(const Matrix& M)
{
if (this->m_capacity != M.capacity())
{
throw std::out_of_range("Input is invalid");
}
for (unsigned int i = 0; i < M.rows(); i++)
{
for (unsigned int j = 0; j < M.cols(); j++)
{
this->m_vec[i + m_cols * j] += M(i, j);
}
}
return *this;
}
template <class T>
Matrix<T> operator+(Matrix<T> M1, Matrix<T>& M2)
{
if (M1.capacity() != M2.capacity())
{
throw std::out_of_range("Input is invalid");
}
return M1 += M2;
}
It compiles just fine, no issues there, but when I try to do a unit test on this, the entire test program just crashes when attempting to chain the +
operator.
Example:
TEST(add, Matrix)
{
Matrix<int> M1 = Matrix<int>(2, 3);
Matrix<int> M2 = { 1, 2, 3, 4 };
Matrix<int> M3 = M2;
Matrix<int> M4 = { 2, 4, 6, 8 };
ASSERT_THROW(M1 + M2, std::out_of_range);
ASSERT_EQ((M2 + M3) == M4, true);
M2 += M3;
M2 = M4 + M4 + M4; // As soon as this line is added, it crashes, without it, test works fine
ASSERT_EQ(M2 == M4, true);
}
Any ideas why it crashes? How can I rewrite my operator overloads so that ´+´ is chainable (and +=
isn't)?
EDIT:
Here is my =
operator (upon request)
template <class T>
void Matrix<T>::operator=(Matrix & M){
T*temp = new T[M.m_capacity];
for(unsigned int j = 0; j < M.m_capacity; j++){
temp[j] = M.m_vec[j];
}
delete[] this -> m_vec;
size_t rows = M.get_m_rows();
size_t cols = M.get_m_cols();
this -> m_rows = rows;
this -> m_cols = cols;
this -> m_vec = new T [rows*cols];
this -> m_capacity = rows*cols;
for(size_t i = 0;i < rows;i++){
for(size_t j = 0;j < cols;j++){
this -> m_vec[i*cols+j] = temp[i*cols +j];
}
}
delete [] temp;
}
EDIT2: Added more context (upon request, header, constructors etc.)
Header:
template <class T>
class Matrix {
public:
// constructor
Matrix(unsigned int n);
Matrix(unsigned int n, unsigned int m);
Matrix();
Matrix(const T n);
Matrix(Matrix &obj);
~Matrix();
Matrix(Matrix &&obj);
Matrix(std::initializer_list<T> l);
// operators
void operator=(Matrix & obj);
T& operator()(unsigned int row, unsigned int col);
Matrix& operator=( Matrix &&obj);
Matrix& operator+=(const Matrix& M)
void operator+=(const T number);
void operator-=(const T number);
void operator-=(Matrix &obj);
void operator*=(const T number);
void operator*=(Matrix &obj);
bool operator==(Matrix & rhs);
private:
std::size_t m_rows;
std::size_t m_cols;
std::size_t m_capacity;
T * m_vec;
};
Copy constructor:
template <class T>
Matrix<T>::Matrix(Matrix &obj){
size_t rows = obj.get_m_rows();
size_t cols = obj.get_m_cols();
this -> m_rows = rows;
this -> m_cols = cols;
this -> m_vec = new T [rows*cols];
this -> m_capacity = rows*cols;
for(size_t i = 0;i < rows;i++){
for(size_t j = 0;j < cols;j++){
this -> m_vec[i*cols+j] = obj(i,j);
}
}
}
Destructor:
template <class T>
Matrix<T>::~Matrix(){
delete [] m_vec;
}
Move constructor (possibly broken)
template <class T>
Matrix<T>::Matrix(Matrix &&obj){
size_t rows = obj.get_m_rows();
size_t cols = obj.get_m_cols();
this -> m_rows = rows;
this -> m_cols = cols;
this -> m_vec = new T [rows*cols];
this -> m_capacity = rows*cols;
m_vec = nullptr;
}
Move assignment (possibly broken)
template <class T>
Matrix<T>& Matrix<T>::operator=(Matrix &&obj){
if (this !=&obj)
{
delete [] m_vec;
obj.m_rows = 0;
obj.m_cols = 0;
obj.m_capacity = 0;
obj.m_vec = nullptr;
}
return *this;
}
Both your move constructor and your move assignment operator do not implement the correct semantics (and it seems you are aware of that), leading to UB somewhere later. (I didn't bother checking exactly where.)
I guess you were assuming that you don't actually call these operators, but that is wrong.
You are calling the move assignment operator at the =
sign of
M2 = M4 + M4 + M4;
because the right-hand side is a prvalue (operator+
returns by-value) which can bind to a rvalue reference.
(Before C++17) You are calling the (possibly elided) move constructor at the second +
in the same line to construct the first parameter of operator+
, because the first +
results in a prvalue.
If you intend to implement the move operations later and you are fine with using the copy implementations instead for the time being, then don't declare the move operations in your class at all. Then the compiler will choose your copy implementations instead.
Additionally, the copy constructor and the copy assignment operator should always take a const
(lvalue) reference as parameter, not a non-const
reference.