Thread: Operator Overloading problem

Code:

#pragma once
#include <vector>
#include <string>
#include <sstream>
#include <deque>
#include <iostream>
#include <iomanip>
using namespace std;

class Matrix
{
private:
	string _name;
	int  _row;
	int  _col;
	double ** _mat;
public:
	//Constructors
	Matrix();
	Matrix::Matrix(const Matrix &m);
	//Deconstructor
	~Matrix();
	//get functions for variables
	string getName(void);
	int getRow(void);
	int getCol(void);
	double ** getPointer(void);
	//set functions for variables
	void setName(string name);
	void setPointer(double ** mat);
	void setRow(int row);
	void setCol(int col);
	//disp matrix 
	void dispMatrix(void);
	//clear matrix
	void clear(void);
	//operators needed for overloading
	Matrix & Matrix::operator = (const Matrix & mMatrix);
};

Code:

#include "Matrix.h"
//default constructor
Matrix::Matrix()
{
	_name = "Unknown";
	_row = 0;
	_col = 0;
	_mat = NULL;
}
//copy constructor call
Matrix::Matrix(const Matrix &m)
{
	_name = m._name;
	_row = m._row;
	_col = m._col;
	_mat = m._mat;
}
//destructor call
Matrix::~Matrix()
{
	delete _mat;
}
//returns the name of a matrix
string Matrix::getName(void)
{
	return _name;
}
//returns the number of rows
int Matrix::getRow(void)
{
	return _row;
}
//returns the number of columns
int Matrix::getCol(void)
{
	return _col;
}
//returns the pointer of data for a matrix
double ** Matrix::getPointer(void)
{
	return _mat;
}
//this sets the name of the string passed to matrixs name
void Matrix::setName(string name)
{
	_name.clear();
	_name = name;
}
void Matrix::setPointer(double ** mat)
{
	_mat = mat;
}
//set the row to row
void Matrix::setRow(int row)
{
	_row = row;
}
//set the col to col
void Matrix::setCol(int col)
{
	_col = col;
}
//this displays the matrix to cout
void Matrix::dispMatrix(void)
{
	int i,j;
	cout << _name << " =" << endl << "\t";
	for(i=0;i<_row;i++)
	{
		for(j=0;j<_col;j++)
		{
			cout << _mat[i][j] << "\t";
		}
		cout << endl;
	}
	cout << endl;
}
//clear the current matrix
void Matrix::clear(void)
{
	_name = "";
	_row = 0;
	_col = 0;
	_mat = NULL;
}
//assignment opererator overloading
Matrix & Matrix::operator = (const Matrix & mMatrix)
{
	int i,j;
	_row = mMatrix._row;
	_col = mMatrix._col;
	_name = mMatrix._name;
	_mat = new double *[_row];
	for(i=0;i<_row;i++)
	{
		_mat[i] = new double [_col];
		for(j=0;j<_col;j++)
		{
			_mat[i][j] = mMatrix._mat[i][j];
		}
	}
	return *this;
}

Code:

// Check for '[' --> assigning new values to matrix
					if (c == '[') 
					{
						if (pos == -1) 
						{	
							getline(cin, restOfLine);
							setCheck = str2matrix(restOfLine,&temp);
							switch(setCheck)
							{
							case -1:
								cout << "Improperly formatted matrix" << endl;
								break;
							case -2:
								cout << "Improperly formatted matrix" << endl;
								break;
							case -3:
								cout << "Dimensions don't match" << endl;
								break;
							case -4:
								cout << "Improperly formatted matrix" << endl;
								break;
							case 1:
								temp.setName(cmd);
								mList.push_back(temp);   // errors here when i push a 2nd time
								break;
							}
						}
						else
						{				
							getline(cin, restOfLine);
							setCheck = str2matrix(restOfLine,&temp);
							switch(setCheck)
							{
							case -1:
								cout << "Improperly formatted matrix" << endl;
								break;
							case -2:
								cout << "Improperly formatted matrix" << endl;
								break;
							case -3:
								cout << "Dimensions don't match" << endl;
								break;
							case -4:
								cout << "Improperly formatted matrix" << endl;
								break;
							case 1:
								mList[pos] = temp;
								break;
							}
							
						}
					}

Code:

//string to matrix form
int str2matrix(string restOfLine, Matrix * temp)
{
	int i,j;
	string tempString;
	deque<string> sList;
	istringstream tempNumString;
	double tempDBL;
	int row = 0;
	int col = 0;
	int lastCol = 0;
	double ** _mat;
	if(*restOfLine.begin() == '[' && restOfLine.back() == ']')
	{
		for(string::iterator sIT = restOfLine.begin(); sIT != restOfLine.end(); ++sIT)
		{
			switch(*sIT)
			{
			case '[':
				//return a -1 if the open bracket isnot at the beginning
				if(distance(restOfLine.begin(), sIT) != 0)
					return -1;
				break;
			case ']':
				//return a -2 if the closed bracket is not at the end
				if(distance(restOfLine.end(), sIT) == 1)
					return -2;
				break;
			case ' ':
				//keep on adding to num of columns
				while(*sIT == ' ')
				{
					++*sIT;
				}
				break;
			case ';':
				if(lastCol == 0)
				{
					row ++;
					lastCol = col;
					col = 0;
				}
				else if(lastCol == col)
				{
					row ++;
					col = 0;
				}
				else
				{
					//this returns a -3 for an error on invalid number of columns
					return -3;
				}
					break;
			default:
				if(*sIT >= 48 && *sIT <= 57)
				{
					while(*sIT >= 48 && *sIT <= 57)
					{
						tempString.push_back(*sIT);
						++sIT;
					}
					--sIT;
					sList.push_back(tempString);
					tempString.clear();
					col ++;
				}
				else
				{
				//returns -4 for error on not a number but a character
					return -4;
				}
			}
		}
		row++;

		_mat = new double *[row-1];
		for(i=0;i<row;i++)
		{
			_mat[i] = new double [col-1];
			for(j=0;j<col;j++)
			{
				tempNumString.str(sList.front());
				tempNumString >> tempDBL;
				sList.pop_front();
				tempNumString.clear();
				_mat[i][j] = tempDBL;
			}
		}
		temp->setPointer(_mat);
		temp->setRow(row);
		temp->setCol(col);
		//return a 1 for completed
		return 1;
	}
	else
		return -2;
}

Alright the above code is my entire class header and implementation. Im not going to post all the main code because its too big so above is where i am getting errors. I have heap errors and other memory problems.

The user should input something like the following

Code:

a = [ 1 2 3; 4 5 6]

Originally Posted by omGeeK

above is where i am getting errors. I have heap errors and other memory problems.

If you insist on using manual memory management, then I suggest that you put this project on hold and practice by writing your own dynamic array class first.

Some comments:

• You have a using directive at file scope in the header.
• Your copy constructor performs a shallow copy, yet your copy assignment operator performs a deep copy.
• Your destructor uses delete where it should use delete[], but then you need to delete[] each inner array too. Furthermore, since your copy constructor does a shallow copy, you are at the risk of double deletion since you don't have say, reference counting.
• Your code is not exception safe.
• Your clear member function can be called after memory has been dynamically allocated, yet it makes no attempt to delete[].
• Your member functions that logically do not change the observable state of the object were not declared const.
• What is the point of exposing the internals of the object with getPointer and setPointer?

The assignment operator is broken also. An assignment operator has two tasks, it has to free resources associated with the current value it holds, and it has to acquire resources required to copy the state of the objects that it is being assigned from. It should also do this in such a way that if an exception was thrown, e.g. due to out of memory, that it doesn't itself leak and ideally doesn't end up having changed anything.
The good news is that there's a really good easy way to ensure perfect operation of an assignment operator. It's called the "Copy and swap idiom". It would make your assignment operator look as simple as this:

Code:

Matrix & Matrix::operator = (const Matrix & other)
{
    Matrix theNewMe(other);
    swap(theNewMe);
    return *this;
}

Of course you're now left with the task of correctly implementing both the copy-constructor, which needs to do the kind of stuff that your broken assignment operator previously did, and the swap method. The copy-constructor only has the one job to do, just copying the state from the object it is being constructed from.

The swap method needs to be what is known as a non-throwing swap. This means that it needs to only do thing that cannot throw exceptions. This means that using new or new[] is out etc. Generally all they do is swap the internal members such as pointers between the two objects.

You should also learn about the "constructor initialisation list" syntax.

Post your updated code soon so that we can see how you're going with this.

Code:

#include "Matrix.h"
//default constructor
Matrix::Matrix()
{
	_name = "Unknown";
	_row = 0;
	_col = 0;
	_mat = NULL;
}
//copy constructor call
Matrix::Matrix(const Matrix &m)
{
	int i,j;
	_row = m._row;
	_col = m._col;
	_name = m._name;
	_mat = new double *[_row];
	for(i=0;i<_row;i++)
	{
		_mat[i] = new double [_col];
		for(j=0;j<_col;j++)
		{
			_mat[i][j] = m._mat[i][j];
		}
	}
}
//destructor call
Matrix::~Matrix()
{
	delete[] _mat;
}
//swap for the copy
void Matrix::swap(Matrix m)
{
        _name = m._name;
	_row = m._row;
	_col = m._col;
	delete _mat;
	_mat = m._mat;
}
//returns the name of a matrix
string Matrix::getName(void)
{
	return _name;
}
//returns the number of rows
int Matrix::getRow(void)
{
	return _row;
}
//returns the number of columns
int Matrix::getCol(void)
{
	return _col;
}
//clear the matrix
void Matrix::clear()
{
	_name = "";
	_row = 0;
	_col = 0;
	delete[] _mat;
}
//this sets the name of the string passed to matrixs name
void Matrix::setName(string name)
{
	_name.clear();
	_name = name;
}
//set the pointer 
void Matrix::setPointer(double ** mat)
{
	_mat = mat;
}
//set the row to row
void Matrix::setRow(int row)
{
	_row = row;
}
//set the col to col
void Matrix::setCol(int col)
{
	_col = col;
}
//this displays the matrix to cout
void Matrix::dispMatrix(void)
{
	int i,j;
	cout << _name << " =" << endl << "\t";
	for(i=0;i<_row;i++)
	{
		for(j=0;j<_col;j++)
		{
			cout << _mat[i][j] << "\t";
		}
		cout << endl;
	}
	cout << endl;
}
//transpose a matrix
Matrix Matrix::transpose(void)
{
	int i,j;
	double temp;
	for(i=0;i<_row;i++)
	{
		for(j=i+1;j<_col;j++)
		{
			 temp=_mat[i][j];
			 _mat[i][j]=_mat[j][i];
			 _mat[j][i]=temp;
		}
	}
	return *this;
}

	
//sets the data of a matrix given a matrix operator overloading
Matrix & Matrix::operator = (const Matrix & mMatrix)
{
	Matrix newMatrix(mMatrix);
	swap(newMatrix);
    return *this;
}

So i was still getting some memory errors when i was trying some funny things like assigning matrices that had been cleared to other matrices and stuff. So i went with the swap idiom you were talking about but seem to have messed it up somewhere.

The main thing is that you just need to get the swap working right now I believe. Currently it's just doing a copy.
Like this:

Code:

void Matrix::swap(Matrix m)
{
    using std::swap;

    swap(_name, m._name);
    swap(_row, m._row);
    // etc...

}

Oh, also remove that delete[] you added inside the assignment operator. What I showed before was literally exactly all that you need, nothing more. Have a read about how the copy-and-swap idiom takes care of both the new allocation and the deallocation of the old data for you.

Once you have that sorted, you have a memory leak to fix because the destructor is not freeing the rows.

Code:

#include "Matrix.h"
//default constructor
Matrix::Matrix()
{
	_name = "Unknown";
	_row = 0;
	_col = 0;
	_mat = NULL;
}
//copy constructor call
Matrix::Matrix(const Matrix &m)
{
	int i,j;
	_row = m._row;
	_col = m._col;
	_name = m._name;
	_mat = new double *[_row];
	for(i=0;i<_row;i++)
	{
		_mat[i] = new double [_col];
		for(j=0;j<_col;j++)
		{
			_mat[i][j] = m._mat[i][j];
		}
	}
}
//destructor
Matrix::~Matrix()
{
	if(this->_mat != NULL)
	{
		for(int i = 0; i < _row; i++)
		{
			if(this->_mat[i] != NULL)
			{
				delete[] _mat[i];
			}
		}
		delete [] _mat;
	}
}
//swap for the copy
void Matrix::swap(Matrix m)
{
	using std::swap;       
	swap(_name, m._name);     
	swap(_row, m._row); 
	swap(_col, m._col);
	swap(_mat,m._mat);
}
//returns the name of a matrix
string Matrix::getName(void)
{
	return _name;
}
//returns the number of rows
int Matrix::getRow(void)
{
	return _row;
}
//returns the number of columns
int Matrix::getCol(void)
{
	return _col;
}
//clear the matrix
void Matrix::clear()
{
	if(this->_mat != NULL)
	{
		for(int i = 0; i < _row; i++)
		{
			if(this->_mat[i] != NULL)
			{
				delete[] _mat[i];
			}
		}
		delete [] _mat;
	}
	_name = "";
	_row = 0;
	_col = 0;
}
//this sets the name of the string passed to matrixs name
void Matrix::setName(string name)
{
	_name.clear();
	_name = name;
}
//set the pointer 
void Matrix::setPointer(double ** mat)
{
	_mat = mat;
}
//set the row to row
void Matrix::setRow(int row)
{
	_row = row;
}
//set the col to col
void Matrix::setCol(int col)
{
	_col = col;
}
//this displays the matrix to cout
void Matrix::dispMatrix(void)
{
	int i,j;
	cout << _name << " =" << endl << "\t";
	for(i=0;i<_row;i++)
	{
		for(j=0;j<_col;j++)
		{
			cout << _mat[i][j] << "\t";
		}
		cout << endl;
	}
	cout << endl;
}
//transpose a matrix
Matrix Matrix::transpose(void)
{
	int i,j;
	if(this->_mat == NULL)
	{
		Matrix mtemp;
		mtemp = *this;
		if(this->_mat != NULL)
		{
			for(int i = 0; i < _row; i++)
			{
				if(this->_mat[i] != NULL)
				{
					delete[] _mat[i];
				}
			}
			delete [] _mat;
		}
		_mat = new double* [mtemp._col-1];
		for(i=0;i<mtemp._col;i++)
		{
			_mat[i] = new double [mtemp._row-1];
		}
		_row = mtemp._col;
		_col = mtemp._row;
		for(i=0;i<_row;i++)
		{
			for(j=0;j<_col;j++)
			{
				 _mat[i][j] = mtemp._mat[j][i];
			}
		}
	}
	return *this;
}
//sets the data of a matrix given a matrix operator overloading
Matrix & Matrix::operator = (const Matrix & mMatrix)
{
		if (this != &mMatrix) // protect against invalid self-assignment
        {
			Matrix temp(mMatrix);
			swap(mMatrix);
        }
        // by convention, always return *this
        return *this;
}

I am still double deleting something some how because i still get heap errors... Man my proffessor is an ass, I just found out last night while I have been trying to do this what the difference is between delete and delete[].. Man i wish i could go back to malloc and free lol

BTW i was reading about std::swap with pointers and I believe what I have is wrong but I read somewhere that it can accept pointers so I just gave that a shot, idk though.

Edit: Also what does delete[] set the pointer to if it doesn't set it to null?

Originally Posted by omGeeK

I am still double deleting something some how because i still get heap errors...
Man i wish i could go back to malloc and free lol

Tsk. New and delete and actually pretty convenient features.

Perhaps the double free errors are related to the code in clear(), and this misunderstanding:

what does delete[] set the pointer to if it doesn't set it to null?

The same thing as free() It doesn't set it to anything, it just deallocates the memory.

Originally Posted by omGeeK

BTW i was reading about std::swap with pointers and I believe what I have is wrong but I read somewhere that it can accept pointers so I just gave that a shot, idk though.

Yes, you can swap the pointers. However, this is wrong:

Code:

void Matrix::swap(Matrix m)

It should be:

Code:

void Matrix::swap(Matrix& m)

Of course, you need to fix the declaration in the class definition too.

By the way, you can implement clear in this way:

Code:

//clear the matrix
void Matrix::clear()
{
    Matrix temp;
    swap(temp);
}

Except that clear sets _name to "" whereas the default constructor sets _name to "Unknown".

Originally Posted by MK27

The same thing as free() It doesn't set it to anything, it just deallocates the memory.

It would typically also invoke the appropriate destructor, if any.

It's looking fairly close to done now.
After correcting the Swap prototype (sorry that's my fault, in haste I totally missed putting the & in there) it should work well enough.

A few things I would still consider looking into, but that don't stop it working, are:

1. Constructor initialisation list syntax. I.e.
   Code:

   Matrix::Matrix() : _row(0), _col(0), //...

2. The destructor could call clear() to remove duplication of code.
3. The second if-statement inside clear is not needed because it is safe to delete NULL. The first one is because you can't iterate over the array if it is NULL.
4. The transpose method should work similarly to the assignment operator in that it should create a new matrix first, then copy the contents over to it in a transposed fashion, and then swap that with the current object. This adds exception safety and removes code duplication. The part of it that has similar code the the destructor should just use the destructor etc.
5. I would remove setRow, setCol and setPointer, as these to me are things that break invariants of the class. If you want to use external data to create a matrix, then the best way to do that would be to create another constructor that takes the number of rows and columns, and perhaps optionally the pointer. This would then also be very useful for your transpose function.
6. setName doesn't need to clear the name first. If you remember from earlier, the assignment operator has two jobs, one of which is to free the resources it currently holds.
7. The string parameter could be passed by const-reference which is more efficient
8. Const-correctness: Marking a function as const means that it can be called on a temporary object or on something that is inside a const variable etc. To do this just put const at the end like this:
   Code:

   string Matrix::getName(void) const

   It will then helpfully give you an error if you accidentally try and make any changes to member variables within that function.

Originally Posted by omGeeK

How would I initially free the data if i went about it the way you were speaking?

You wouldn't. The point of the swapping technique is to produce a temporary object with required content THEN swap the temporary with the original (*this) and return. Since the temporary will be destroyed, and it holds the original content of *this, all is cleaned up safely.

Originally Posted by omGeeK

My solution is above, are there any safety problems with it?

Imagine what would happen, inside your loop, if the line "_mat[i] = new double[_col]" failed second time through the loop. In what state would the matrix be in? Would it even be usable? Could the destructor of Matrix safely be invoked to clean up your matrix?

The basic issue is that you need to assume any operation in your code may fail, and structure your code so it will cope with ANY failure. You are assuming that every operation succeeds, and therefore structuring your code in a manner that means any failure will cause unrecoverable chaos.

One other weakness with your code is that all identifiers beginning with an underscore are reserved for use by the implementation in the global namespace. While that is not necessarily a problem for your code, it is still generally inadvisable to use such identifiers.

Because your swap is a non-static member function, and can only be used in the form "some_object.swap(some_other_object)" rather than as a naked function call.

That test for self-assignment is bad practice. And it is also unnecessary to protect against self-assignment if you properly use the swap technique.