Like Tree4Likes

Operator Overloading problem

This is a discussion on Operator Overloading problem within the C++ Programming forums, part of the General Programming Boards category; Code: #pragma once #include <vector> #include <string> #include <sstream> #include <deque> #include <iostream> #include <iomanip> using namespace std; class Matrix ...

  1. #16
    Registered User
    Join Date
    Sep 2010
    Location
    Boston, MA
    Posts
    97
    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]
    Last edited by omGeeK; 04-05-2012 at 01:01 PM.

  2. #17
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,448
    Quote 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?
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #18
    Registered User
    Join Date
    Sep 2010
    Location
    Boston, MA
    Posts
    97
    Thanks fixed a bunch of stupid errors. I have been staring at this code for a while and clearly not really paying attention.

  4. #19
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,295
    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.
    Last edited by iMalc; 04-05-2012 at 02:12 PM.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  5. #20
    Registered User
    Join Date
    Sep 2010
    Location
    Boston, MA
    Posts
    97
    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.
    Last edited by omGeeK; 04-05-2012 at 03:05 PM.

  6. #21
    Registered User
    Join Date
    Sep 2010
    Location
    Boston, MA
    Posts
    97
    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()
    {
    	delete[] _mat;
    }
    //swap for the copy
    void Matrix::swap(Matrix m)
    {
    	_name = m._name;
    	_row = m._row;
    	_col = m._col;
    	_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)
    {
    	delete[] _mat;
    	Matrix newMatrix(mMatrix);
    	swap(newMatrix);
        return *this;
    }
    UPDATED still not working though

  7. #22
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,295
    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.
    Last edited by iMalc; 04-06-2012 at 12:53 AM.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  8. #23
    Registered User
    Join Date
    Sep 2010
    Location
    Boston, MA
    Posts
    97
    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?
    Last edited by omGeeK; 04-06-2012 at 08:32 AM. Reason: updated code

  9. #24
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Quote Originally Posted by omGeeK View Post
    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.
    Last edited by MK27; 04-06-2012 at 08:52 AM.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  10. #25
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,448
    Quote 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".

    Quote 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.
    MK27 and iMalc like this.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  11. #26
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,295
    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.
    Last edited by iMalc; 04-06-2012 at 01:29 PM.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  12. #27
    Registered User
    Join Date
    Sep 2010
    Location
    Boston, MA
    Posts
    97
    Code:
    //transpose a matrix
    Matrix Matrix::transpose(void)
    {
    	int i,j;
    	if(this->_mat != NULL)
    	{
    		Matrix mtemp(*this);
    		this->clear();
    		_row = mtemp._col;
    		_col = mtemp._row;
    		_mat = new double *[_row];
    		for(i=0;i<_row;i++)
    		{
    			_mat[i] = new double [_col];
    			for(j=0;j<_col;j++)
    			{
    				 _mat[i][j] = mtemp._mat[j][i];
    			}
    		}
    	}
    	return *this;
    }
    The only problem I have understanding your comments malc is the transpose. How would I initially free the data if i went about it the way you were speaking? My solution is above, are there any safety problems with it?

    Oh and thanks btw to you malc and laserlight, your help is much appreciated.

  13. #28
    Registered User
    Join Date
    Jun 2005
    Posts
    6,208
    Quote Originally Posted by omGeeK View Post
    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.

    Quote Originally Posted by omGeeK View Post
    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.
    Right 98% of the time, and don't care about the other 3%.

  14. #29
    Registered User
    Join Date
    Sep 2010
    Location
    Boston, MA
    Posts
    97
    Alright, Ill have to work on that but I have another quick question

    Code:
    //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);     //says wont allow type const Matrix only Matrix &
        }
    	// by convention, always return *this
    	return *this;
    }
    Code:
    //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);
    }
    Why is the compiler giving me problems on the swap saying its invalid argument?

  15. #30
    Registered User
    Join Date
    Jun 2005
    Posts
    6,208
    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.
    Right 98% of the time, and don't care about the other 3%.

Page 2 of 3 FirstFirst 123 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Operator Overloading problem
    By FMan in forum C++ Programming
    Replies: 2
    Last Post: 10-26-2008, 12:37 PM
  2. operator overloading problem
    By salmansalman in forum C++ Programming
    Replies: 3
    Last Post: 08-21-2008, 07:00 AM
  3. Problem with overloading an operator.
    By apacz in forum C++ Programming
    Replies: 3
    Last Post: 08-04-2006, 10:00 AM
  4. operator overloading problem~
    By black in forum C++ Programming
    Replies: 6
    Last Post: 07-20-2004, 09:04 AM
  5. Problem with Operator Overloading
    By bench386 in forum C++ Programming
    Replies: 1
    Last Post: 03-29-2004, 12:39 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21