Not logged in, Join Here! or Log In Below:  
 
News Articles Search    
 

 Home / General Programming / memory leak? Account Manager
 
Archive Notice: This thread is old and no longer active. It is here for reference purposes. This thread was created on an older version of the flipcode forums, before the site closed in 2005. Please keep that in mind as you view this thread, as many of the topics and opinions may be outdated.
 
muhd

February 08, 2005, 04:29 PM

Hi, i've been subjected to java for too long and coming back to c++ has been a bit confusing in some areas. I'm writing a raytracer but first I'd like to make sure my math library is memory leak free, or as little as possible. Is this code ok to use and is it being correctly used. I know not to return references to pointers allocated by new inside a function because there is no way to delete them but Im not sure about this:


  1.  CMatrix* CMatrix::mAdd(CMatrix &matrix1, CMatrix& matrix2)
  2. {
  3.         if(matrix1.m_rows != matrix2.m_columns || matrix1.m_columns != matrix2.m_rows)
  4.         {
  5.                 cout << "Not a square matrix!!! must be nxn + nxn matrix " << endl;
  6.                 return this;
  7.         }
  8.        
  9.         // create a new matrix using dimensions of matrix1 and matrix2 (pass by reference)
  10.         CMatrix* ReturnMatrix = new CMatrix(matrix1.m_rows, matrix2.m_columns);
  11.         for(int j = 0; j < matrix1.m_rows_cols; j++)
  12.         {                      
  13.                 ReturnMatrix->m_matrix[j] = (matrix1.m_matrix[j] + matrix2.m_matrix[j]);
  14.         }
  15.  
  16.         return ReturnMatrix;
  17. }


so that code lies inside my CMatrix.cpp, now if i do this(note: matrix1 & matrix2 are both of type CMatrix*):

  1.  CMatrix* ResultMatrix = matrix1->mAdd(*matrix1, *matrix2); // ResultMatrix now points to a new matrix (ReturnMatrix) which is the sum of the two matricies added together. i.e. ResultMatrix->ReturnMatrix = 3x3Matrix;
  2.  
  3. delete ResultMatrix; // Does this delete the ReturnMatrix or just ResultMatrix?


is this the correct way to go about this? Any hints, tips and pointers :) would be greatly appreciated.


thank you.

muhd


 
Reedbeta

February 08, 2005, 05:21 PM

Generally, I would return by value. This can cause an extra temporary to be created, but in many cases the compiler can get rid of it through RVO (return value optimization).

You don't want to return references (or pointers) to local variables, for obvious reasons, but returning pointers to memory allocated with new is no better, because you then cannot write complex expressions like (a*b) + (c*d), as there will be no way to delete the temporaries created.

 
Victor Widell

February 08, 2005, 07:03 PM

The possible performance gain you would get by returning by reference will be lost by the allocation and deallocation.

Also, this is SUICIDAL coding style. If you really MUST return a newly allocated memmory, use a smart pointer instead.

 
muhd

February 08, 2005, 07:30 PM

excellent thanks for the info. Ive seen the error of my ways and am currently correcting.

thanks for the input.

muhd

 
juhnu

February 08, 2005, 11:05 PM

consider overloading the + and * operators instead of having add/mul methods in your classes. makes the code look a bit cleaner..

  1.  
  2.    matrix1 += matrix2;
  3.    CMatrix matrix3 = matrix1 + matrix2;
  4.  




juhani

 
mrobin604

February 10, 2005, 01:42 PM

Any particular reason your matrix class uses arbitrarily size matrices? I would think you'd want a 4x4 mat class for transforms even if you have a mxn mat class for other purposes. The arbitrary dimensions will result in slower code and possibly memory fragmentation if you're allocating arbitrary sized mats on the heap.

Oh, and what they said about the return values. Return by value, or you could do something like:

  1. CMatrix&  CMatrix::Add(CMatrix &mat1, CMatrix &mat2);


where the result of mat1*mat2 are put into the *this matrix, and you could end the method with

  1. return *this;


-marsh

 
muhd

February 15, 2005, 08:11 AM

Thanks for the current suggestions. I've taken out all those nasty pointers and references to nothing but im left with a problem. Perhaps it would be easier if I showed the code first.

The CMatrix Header File

  1. public:
  2.  
  3.         /* Constructors, Deconstructors & Overloaded Operators */
  4.         CMatrix(void);
  5.         CMatrix(unsigned int rows, unsigned int columns);
  6.  
  7.         ~CMatrix(void);
  8.         CMatrix& operator =(const CMatrix&);
  9.         CMatrix operator +(const CMatrix&);
  10.  
  11. ...........
  12.  
  13. private:       
  14.         float* m_matrix; // this is what I use to store the matrix dimensions.
  15.         int m_rows_cols;
  16.         int m_rows;
  17.         int m_columns;
  18.         char* name;
  19.  
  20.  


And here is part of the source file.

  1.  
  2.  
  3. CMatrix::CMatrix(unsigned int rows, unsigned int columns):m_rows(rows), m_columns(columns), m_rows_cols(rows*columns)
  4. {
  5.         // Create a new 1D float matrix based on the size of row x col (i.e. 3x3 matrix = matrix[9];
  6.         m_matrix = new float[m_rows_cols];
  7.  
  8.         // Assign the matrix a name - for debugging purposes.
  9.         name = new char[12];
  10.         name = "Matrix";
  11.  
  12.         // go through each element in the matrix and set it to = 0.0;  
  13.         for(int j = 0; j < m_rows_cols; ++j)
  14.                 *(m_matrix+j) = 0.0;
  15.  
  16.         cout << "ttFINISHED CALLING CMATRIX CONSTRUCTOR" << endl;
  17. }
  18.  
  19. CMatrix::~CMatrix()
  20. {      
  21.         cout << "ttDELETING " << this->name << endl;
  22.         delete this->name;
  23.         delete [] this->m_matrix;
  24.         cout << "tt****FINISHED CALLING CMATRIX DECONSTRUCTOR" << endl;
  25.  
  26. }
  27.  
  28. CMatrix CMatrix::operator +(const CMatrix &add)
  29. {
  30.                 if(this->m_rows != add.m_columns || this->m_columns != add.m_rows)
  31.         {
  32.                 cout << "Not a square matrix!!! must be nxn + nxn matrix " << endl;
  33.                 return *this;
  34.         }
  35.         CMatrix R(this->m_rows, add.m_columns);
  36.         for(int j = 0; j < this->m_rows_cols; j++)
  37.         {                      
  38.                 R.m_matrix[j] = (this->m_matrix[j] + add.m_matrix[j]);
  39.         }
  40.         R.outputMatrix(R);     
  41.         return R;
  42. }
  43.  
  44.  
  45.  


and finally a little test application

  1.  
  2. CMatrix oldmatrix1(3,3); // inital values for these matrices are all zeros
  3. CMatrix oldmatrix2(3,3);
  4.  
  5. oldmatrix1.setIdentity(); // set both to identity matrices
  6. oldmatrix2.setIdentity();
  7.  
  8. // create a new matrix and have it equal the sum of those two matrices
  9. CMatrix Matrixnew = oldmatrix1 + oldmatrix2;
  10.  
  11.  



Ok as you can see inside my overloaded + I am calling the constructor to create a new CMatrix object named R. Inside the constructor is a call to new which grabs the rows and columns values and creates a float array based on those values. I then delete that new float array in the destructor.

When I run the test application Matrixnew is output to screen and all the values are corrupted i.e. dont have values. Matrixnew should display 200020002 but it doesnt. Im thinking this is because when I call my CMatrix R inside operator + it creates a new float array then does all the calculations then when it comes to the end of the operator +, R goes out of scope and calls R's destructor which deletes the nicely calculated float array. Matrixnew now has no float array and so outputs rubbish to screen.

So my question is, how do i get this to work? or is there something I am doing extremely wrong? I want to be able to enter stupid values for CMatrix like CMatrix(35,35) and have it create a 35x35 Matrix so I need dynamica allocation but i cant get this to work. Any ideas, help or anything would be appreciated.

Thank you.

muhd

 
juhnu

February 15, 2005, 08:30 AM

might be your assignment operator is not working?


juhani

 
muhd

February 15, 2005, 08:37 AM

i have a copy constructor and it looks like this (sorry forgot to put it in previous post)

  1.  
  2. CMatrix& CMatrix::operator =(const CMatrix )
  3. {
  4.        
  5.         this->name = copy.name;
  6.         this->m_columns = copy.m_columns;
  7.         this->m_rows = copy.m_rows;
  8.         this->m_rows_cols = copy.m_rows_cols;
  9.         this->m_matrix = copy.m_matrix;
  10.        
  11.        
  12.         return *this;
  13. }
  14.  

is there something i am doing wrong here?

thank you

muhd

 
z80

February 15, 2005, 11:13 AM

Just copying name and m_matrix in your assignment operator is strange since they're pointers. You should probably create a new array for name and m_matrix and copy the data.

Try something like:

  1.  
  2. // Clone matrix data
  3. copy.m_matrix = new float[m_row_cols];
  4. memcpy(copy.m_matrix, m_matrix, m_row_cols * sizeof(float));
  5.  
  6. // Clone name
  7. if (name)
  8. {
  9.    copy.name = new char[strlen(name) + 1];
  10.    strcpy(copy.name, name);
  11. }
  12. else
  13.    copy.name = NULL;
  14.  


Also your ctor/dtor look strange (look at the two 'COMMENT'-lines):

  1.  
  2. CMatrix::CMatrix(unsigned int rows, unsigned int columns):m_rows(rows), m_columns(columns), m_rows_cols(rows*columns)
  3. {
  4.         // Create a new 1D float matrix based on the size of row x col (i.e. 3x3 matrix = matrix[9];
  5.         m_matrix = new float[m_rows_cols];
  6.  
  7.         // Assign the matrix a name - for debugging purposes.
  8.         name = new char[12];
  9.         name = "Matrix"; // COMMENT: Hmm this is strange!? Perhaps strcpy(name, "Matrix"); would be better
  10.  
  11.         // go through each element in the matrix and set it to = 0.0;  
  12.         for(int j = 0; j < m_rows_cols; ++j)
  13.                 *(m_matrix+j) = 0.0;
  14.  
  15.         cout << "ttFINISHED CALLING CMATRIX CONSTRUCTOR" << endl;
  16. }
  17.  
  18. CMatrix::~CMatrix()
  19. {      
  20.         cout << "ttDELETING " << this->name << endl;
  21.         delete this->name; // COMMENT: Shouldnt this be delete[] this->name?
  22.         delete [] this->m_matrix;
  23.         cout << "tt****FINISHED CALLING CMATRIX DECONSTRUCTOR" << endl;
  24.  
  25. }
  26.  

 
Danny Chapman

February 15, 2005, 12:10 PM


muhd wrote: is there something i am doing wrong here?


0. Your operator= should (must?) take a const reference, not a const value, argument.

1. Both the original and the copy will end up with pointers to the same memory (name and m_matrix). When one gets deleted the other will point to stale memory. When the second gets deleted the destructor will attempt to delete stale memory.

2. So, your copy constructor should really copy (and allocate for) what the original pointers point to. If you do this, beware of the situation where the caller writes "a = a" (directly or indirectly - could be "b = a" where b is a reference to a etc). So your copy constructor should check if "this == copy" and return immediately if so.

3. you could use std::string and std::vector to store the internal data - would insulate you somewhat from these kind of errors.

 
muhd

February 15, 2005, 06:45 PM

thanks for all the feedback so far youve been a huge help. Java really hides all this "good stuff" away from you and I really think we (at my uni) should be learning this instead of relying on the jvm to do it for me. Anyway im getting there, slowly :)

Ive added to the copy ctor what youve told me although its slightly different to your implementation z80. Thank you for your comments and input.

The problem I am having now is that ive implemented the copy ctow and when i do something like

  1.  CMatrix Matrixnew = newmatrixobject;


my copy ctor isnt being called therefor Matrixnew.m_matrix points to newmatrixobject.m_matrix which is the trouble i was having before. The new copy ctor when use like this works.

  1.  CMatrix Matrixnew(3,3);
  2. Matrixnew = newmatrixobject;
  3.  


so is there anyway to get round this problem of having to define CMatrixnew(r,c) before I can assign it to a object?

here is the modified copy ctor

  1.  
  2.  
  3. CMatrix& CMatrix::operator =(const CMatrix& copy  )
  4. {
  5.         // check for self assignment
  6.         if(this == & copy){
  7.                 cout << " copy CONSTRUCTOR CALLED-SELF ASSIGNMENT ERROR" << endl;
  8.                 return *this;
  9.         }
  10.  
  11.         this->m_columns = copy.m_columns;
  12.         this->m_rows = copy.m_rows;
  13.         this->m_rows_cols = copy.m_rows_cols;
  14.  
  15.         // Clone matrix data
  16.        
  17.         memcpy(this->m_matrix, copy.m_matrix, copy.m_rows_cols * sizeof(float));
  18.  
  19.         // Clone name
  20.         if (name)
  21.         {
  22.                 strcpy(name,copy.name);
  23.         }
  24.         else
  25.          name = 0;
  26.        
  27.         cout << "tCOPY CONSTRUCTOR CALLED" << endl;
  28.         return *this;
  29. }
  30.  
  31.  


again thank you for your help and sorry for troubling you.

muhd

 
Reedbeta

February 16, 2005, 01:50 AM

What you've implemented there is an assignment operator, not a copy constructor. The copy constructor would have prototype:

CMatrix::CMatrix (const CMatrix& copy)

By the way, in the assignment operator you should delete the old matrix before creating the new one. Rule of thumb: the assignment operator should contain the code of the destructor followed by the code of the copy constructor.

 
Sp0rtyt

February 16, 2005, 02:05 AM

If you're at a real accredited university then you should know how to RTFB

 
muhd

February 16, 2005, 06:59 AM

Thank you for your replies Its all working now :)

muhd

 
gaby

February 16, 2005, 08:59 AM

also note ... if am not mistaken .. that the check you perform to see if the matrices are square would also work on matrices NxM and MxN ..
do you want that ?

 
This thread contains 16 messages.
 
 
Hosting by Solid Eight Studios, maker of PhotoTangler Collage Maker.