Operator overloading problem, need help!

[fairburn]

I'm attempting to make a class which represents a matrix with a one dimensional array:

using namespace std;
typedef double MatrixItem;

// Matrix is structured in a one-dimensional array such that an item at 
// index (i, j) is represented by data[i * columns + j]
class Matrix
{
	public:
		Matrix(int row_size, int col_size);
		~Matrix();

		// Either set element at index (i, j) or directly at a specific
		// location in array
		void setElement(int i, int j, double value);
		void setData(int index, double value);

		// Return data set in above methods
		MatrixItem getElement(int i, int j) const;
		MatrixItem getData(int index) const;

		// Overload + and = operators
		Matrix operator+(const Matrix& orig) const;
		void operator=(const Matrix& orig);

	private:
		// Size of memory to be allocated for array;
		int size;

		// Dimensions of matrix
		int rows;
		int columns;

		// Array which stores elements of matrix
		MatrixItem *data;
};

So far, everything works EXCEPT for the + operator. Here are the overload definitions and my test of the class:

Matrix Matrix::operator+(const Matrix& orig) const
{
	Matrix sum(rows, columns);
	for(int i = 0; i < rows; i++)
		for(int j = 0; j < columns; j++)
			sum.data[i*columns+j] = data[i*columns+j] + orig.data[i*columns+j];

	return sum;
}

void Matrix::operator=(const Matrix& orig)
{
	for(int i = 0; i < orig.rows; i++)
		for(int j = 0; j < orig.columns; j++)
			data[i*columns+j] = orig.data[i*columns+j];
}

int main(void)
{
	int a;
	Matrix m(3, 4);
	m.setElement(1, 1, 5.0);
	Matrix n(3, 4);
	
	n = (m+m); // causes issues, without this line the program executes
	n = m; // works fine

	cout << n.getElement(1, 1); // check that n is equal to m at (1, 1)
	cin >> a; // pause
	return 0;
}

Now, this builds without any errors or warnings (Visual Studio), but when I try to run the executable, a window comes up telling me that the program is causing an assertion failure (image link: http://i.imgur.com/mir6J.jpg ). This is frustrating me to no end because hours of tinkering with the operator+ later, I still get the same error.

What can I do to make this operator overload work correctly? I will post more code if needed, I only included this much for conciseness.

[therockon7throw]

Hi

Why do you define the operator+ this Matrix operator+(const Matrix& orig) const; way ?

and not

this Matrix operator+(const Matrix& orig) ; way ?

and why this n = (m+m) ; way and not just n = m+m ;

and operator = cant be that way void operator=(const Matrix& orig);

it must be Matrix& operator=(const Matrix& orig);


a possible operator=

Matrix& Matrix::operator=(const Matrix& orig){
	   if (this == &orig)
		   return *this;
	   if( row != orig.row || columns != orig.columns ){
	       // this must be clean
		   // and reallocate
	   }

	   //here copy value

	   //and  return 
	   return *this;
	}

And you must implement copy-constructor, Matrix(const Matrix& orig), because you are dealing with pointers and dynamic members

[Peter87]

Are you sure that the problem is not in the constructor or destructor?

The class is a bit dangerous because all operations assumes the other matrix is of the same dimensions.

therockon7throw wrote:

Why do you define the operator+ this Matrix operator+(const Matrix& orig) const; way ?
and not
this Matrix operator+(const Matrix& orig) ; way ?

If he do it that way he can't use a const objects on the left hand side of operator+.

therockon7throw wrote:

and operator = cant be that way void operator=(const Matrix& orig);
it must be Matrix& operator=(const Matrix& orig);

Returning a reference from operator= is kind of a convention but you don't have to do it that way.

[therockon7throw]

No idea how he implements constructor or destructor, we have no source code !

Peter87

Returning a reference from operator= is kind of a convention but you don't have to do it that way

What if the sizes are not equal and the current object, this, must be cleaned and reallocated first ? If you do not return a reference, you go into troubles...


left hand side of operator+ points always to the current object, this, if the operator +, - , / , * are overloaded as methods, otherwise it is not the case

[Vins3Xtreme]

you should implement first the 'compound operators' (+= , -= , *= , /= , ecc.) , and then use them with the 'normal' operators' (+ , - , * , / , ecc.). An example:

Matrix& Matrix::operator+= (const Matrix& operand){  //compound operand for '+='
    if(rows != operand.rows || columns != operand.columns) return *this;
    MatrixItem *buffer1=data, *buffer2=operand.data;
    for(int i=rows -1;i>0;--i){
        for(int j=columns - 1 ; j >0 ; --j)
            (*buffer1++) += (*buffer2++);
    }
    return *this;
}

 //the 'operator+' is implemented easly
Matrix Matrix::operator+(const Matrix& operand)const{  //the return value must be 'Matrix' and not 'Matrix&'
    Matrix sum(*this);  //you must define also the 'copy costructor' of Matrix
    sum+=operand
    return sum;
}

// Copy Costructor
Matrix::Matrix(const Matrix& operand){
    rows=operand.rows;
    columns=operand.columns;
    data=new MatrixItem[rows*columns];
    MatrixItem *buffer1=data, *buffer2=operand.data;
    for(int i=rows -1;i>0;--i){
        for(int j=columns - 1 ; j >0 ; --j)
            (*buffer1++) += (*buffer2++);
}

//the Assignment operator
Matrix& Matrix::operator=(const Matrix& operand){
    if(this == &operand) return *this;
    Matrix replaceMatrix(operand);
    Swap(replaceMatrix);    
    return *this;
}

//the Swap() function
void Matrix::Swap(Matrix& operand){
    int temp=rows;
    rows=operand.rows;
    operand.rows=temp;
    temp=columns;
    columns=operand.columns;
    operand.columns=temp;
    MatrixItem *tData=data;
    data=operand.data;
    operand.data=tData;
}

the Swap() function, in this case, is used for the deallocation of the 'old value'.

[fairburn]

The const is in operator+ because it returns a new matrix and doesn't change any members of an existing object. I changed operator= to this:

Matrix& Matrix::operator=(const Matrix& orig)
{
	if(rows != orig.rows || columns != orig.columns)
	{
		delete[] data;

		rows = orig.rows;
		columns = orig.columns;
		size = rows*columns + columns;
		data = new MatrixItem[size];
	}

	for(int i = 0; i < orig.rows; i++)
		for(int j = 0; j < orig.columns; j++)
			data[i*columns+j] = orig.data[i*columns+j];

	return *this;
}

But that didn't fix the issue, I already got the expected output from it. My problem is at runtime because it builds without error...and it has something to do with operator+ but I can't find the bug for the life of me. It's related to memory allocation because if I add delete[] data; to the top of operator+ I don't get the window popping up, but obviously I don't get the correct output because the original data is deleted and I can't add to it...

[therockon7throw]

@Vins3Xtreme

Could you explain me what are you doing here (*buffer1++) += (*buffer2++); in your Copy Constructor ?

Why are you using swap, exchanging the datas, in your assignment operator implementation ? What for a logic behind ?

You should reconsider and re-code both your copy constructor and assigment operator implementatios

[Vins3Xtreme]

@therockon7throw:
Sure. I use (*buffer1++) += (*buffer2++); because the value 'rows' or 'columns' can be the MAX positive value of a 'int' type (signed int max positive value: 0x7FFFFFFF), i can't use (rows*columns) because can be overflow, then i use a pointer for this reason.
Then i use Swap() to exchange the value of 2 matrix , in the case of assignment operator, the function swap the 2 matrix data array, so when after returning the function, the local variable 'replaceMatrix' is destroyed calling its destructor and the old values are deallocated.

[therockon7throw]

@fairburn

You must implment copy-constructor otherwise it wont work!!

Is not the size of your matrix equal size = rows*columns ?? why you use in assigment operator size = rows*columns + columns; ??

good nigth

[fairburn]

Yes, the matrix itself is (row x columns), but because of the way I'm representing it with a one dimensional array, element (i, j) is located at index [i * columns + j]. So for example in a 5x5 matrix, element (5, 5) is at [5 * columns + 5] = [5*5 + 5] = 30, which is higher than 5*5. Unless my logic is flawed, I kind of rushed through that calculation and haven't looked back at it because I've been too busy pulling my hair out.

Edit: Here's my constructor and destructor, I still haven't implemented a copy constructor though, that may resolve this...

Matrix::Matrix(int row_size, int col_size)
{
	rows = row_size;
	columns = col_size;
	size = rows*columns + columns;
	data = new MatrixItem[size];

	// Initialize each element of matrix to (i - j)
	for(int i = 0; i < rows; i++)
		for(int j = 0; j < columns; j++)
			data[i*columns+j] = (i - j);
}

//-- Destructor: deallocate memory --//
Matrix::~Matrix()
{
	delete[] data;
}

[o1vk4iN6]

You shouldn't be modifying an environment in a header file, such as putting "using namespace std". If someone who isn't using std wants to use the header but the namespace conflicts with their own classes.

Why do you define MatrixItem outside of the class, you can define it inside of the class, again if someone wants to use it they can just do Matrix::MatrixItem.

You also reverted back to using double rather than MatrixItem in some of your functions.

// remove
// using namespace std;

class Matrix
{
	public:
                typedef double MatrixItem; // moved

		Matrix(int row_size, int col_size);
		~Matrix();

		void setElement(int i, int j, MatrixItem value); // use MatrixItem here instead
		void setData(int index, MatrixItem value); // MatrixItem here too

		// Return data set in above methods
		MatrixItem getElement(int i, int j) const;
		MatrixItem getData(int index) const;

		Matrix operator+(const Matrix& orig) const;
		void operator=(const Matrix& orig);

	private:

		int size;

		int rows;
		int columns;

		MatrixItem *data;
};

Edit:

Your size calculation is wrong in your constructor, it's simply:

size = rows * columns;

size = rows*columns + columns; // has an extra row 

[roberts]

and why this n = (m+m) ; way and not just n = m+m ;

While I agree those parens are unnecessary, I tend to use a lot of parens in my code. It helps me to ensure the order of operations if I have any doubts and make the code more mathematically readable.

AFAIK, adding parens does not affect code generation, unless they are ill-placed and affect the order of operations. Compare:

y = x ^ 2 + z

versus

y = x ^ (2 + z)

@OP, check to make sure you're staying within bounds. Most assertion failures on memory arrays are often caused by reading past or before the bounds of the memory block. If you have access to a debugger, attempt to limit the point of failure to a specific line and examine all the variables in play to see if any are OOR (out of range).

Yes, the matrix itself is (row x columns), but because of the way I'm representing it with a one dimensional array, element (i, j) is located at index [i * columns + j]. So for example in a 5x5 matrix, element (5, 5) is at [5 * columns + 5] = [5*5 + 5] = 30, which is higher than 5*5. Unless my logic is flawed, I kind of rushed through that calculation and haven't looked back at it because I've been too busy pulling my hair out.

That is flawed.

A 5 x 5 matrix will be 25, regardless of whether it is represented one or two dimensions. The only thing that would affect the size beyond that is the size of the intrinsic data used to define the data each cell holds.

A 5 x 5 matrix of chars is 25 bytes in size, where as a 5 x 5 matrix of doubles is 200 bytes in size. Unless you're using the other 5 items in your calc for internal use, simply calc the size of the array as 5 x 5 * sizeof(MatrixItem) and nothing more.

[fairburn]

@roberts: I feel a little slow, my logic on that calculation was clearly off because the index starts at 0 and not 1 -_-

@xerzi: I only intended to use MatrixItem as a return type and for members of the class, so reverting back to double at some points was done intentionally. Also I put the typedef outside of the class on purpose so I wouldn't have to keep typing Matrix::, but I understand that by convention it is better to place it inside the class.

I actually got operator+ to work by changing the definition to

Matrix Matrix::operator+(const Matrix& orig) const
{
	Matrix new_matrix(*this);
	Matrix sum(*this);
	
	for(int i = 0; i < orig.rows; i++)
		for(int j = 0; j < orig.columns; j++)
			sum.data[i*columns+j] = new_matrix.data[i*columns+j] + orig.data[i*columns+j];
	return sum;
}

[therockon7throw]

Hi
@Vins3Xtrem

Vins3Xtreme (41) Feb 13, 2012 at 1:03am

Sure. I use (*buffer1++) += (*buffer2++); because the value 'rows' or 'columns' can be the MAX positive value of a 'int' type (signed int max positive value: 0x7FFFFFFF), i can't use (rows*columns) because can be overflow, then i use a pointer for this reason.

It is totally false, you cant do such thing in a copy constructor, a copy contructor is a kind of constructor which takes a const reference to an object of the class itself. So how could you use += for an object element that you just created. what is the value of the new created elements that you are adding with the elements of the orig ????

Then i use Swap() to exchange the value of 2 matrix , in the case of assignment operator, the function swap the 2 matrix data array, so when after returning the function, the local variable 'replaceMatrix' is destroyed calling its destructor and the old values are deallocated.

The assigment operator= returns a reference, so nothing is destroyed, you cant implement the assigment operator that way, what you are doing is exchanging the object.

[therockon7throw]

Hi
@fairburn

Yes, the matrix itself is (row x columns), but because of the way I'm representing it with a one dimensional array, element (i, j) is located at index [i * columns + j]. So for example in a 5x5 matrix, element (5, 5) is at [5 * columns + 5] = [5*5 + 5] = 30, which is higher than 5*5. Unless my logic is flawed, I kind of rushed through that calculation and haven't looked back at it because I've been too busy pulling my hair out.

when you calculate the offset of an element by [i * columns + j] it'is done like following:

for first row ->
[ 0 * columns + j] = just j
for second row -> [ 1 * columns + j] = columns + j

so you do not need to allocate (row x columns) + columns, but just (row x columns)

And for adding and substraction you do not need two loops, use just one loop like following

Matrix Matrix::operator+(const Matrix& orig) const
{
	//Matrix new_matrix(*this); you dont need this one
	//Matrix sum(*this); you should not do that way
          Matrix sum(rows, colunms);
	
	for(int i = 0; i < size; i++)
			sum.data[i] = data[i] + orig[i];
	return sum;
}

Matrix Matrix::operator-(const Matrix& orig) const
{
	//Matrix new_matrix(*this); you dont need this one
	//Matrix sum(*this); you should not do that way
          Matrix sum(rows, colunms);
	
	for(int i = 0; i < size; i++)
			sum.data[i] = data[i] - orig[i];
	return sum;
}

Please overload the operator[] , otherwise you cant access private members , for example youcant access data, in this orig.data[i] way

[fairburn]

@therockon7throw: I actually noticed last night that I wouldn't need the two matrices in operator+ and already changed that. However, why should I not use Matrix sum(*this);?

[therockon7throw]

Hi

fairburn

However, why should I not use Matrix sum(*this);?

It is a matter of taste, and when you overload operators you make it easy to apply operation on your class-objects. For Example which one of the following is easy to code, to read and to understand

First version

Matrix a(3,3), b(3,3 ), c(3,3 ), d(3,3);

// do some assigments to a , b and c

d =  ( a.sum(b) ).sum( c) ;

Second version

Matrix a(3,3), b(3,3 ), c(3,3 ), d(3,3);

// do some assigments to a , b and c

d =   a + b + c ;

and worse cases

d =  ( a.multply( b ) + c.sub( a )  ).sum( c.multiply( b ) ) ; 

//or just
d =  a * b + c - a + c *b ;

You see