Program Freezes on malloc...

[NickPaul]

Hey all, the following code is run probably 15 times before my program freezes, the program gets into this function but never comes out lol CodeBlocks returns this:

Process terminated with status -2147483641 (2 minutes, 8 seconds)

If that helps at all lol and here is the code:

 Matrix::Matrix(int rows, int columns)
{

    cout << "In Construct" << endl;

    nRows = rows;
    nCols = columns;

    pMatrix = (float**)malloc(rows * sizeof(*pMatrix));
	for(int i = 0; i < rows; i++)
	{
		pMatrix[i] = (float*)malloc(columns * sizeof(**pMatrix));
	}

	cout << "End Construct" << endl;

}

Anyway, I'm super new to C++ and especially memory management lol tonight I realised I had a massive memory leak, I'm pretty sure this Matrix object was the cause of the problem since I was never "freeing" pMatrix. Anyway I added a destructor to free the allocated memory. That stuffed my program up because every time I passed a matrix object (a copy of the object) the function would deconstruct the object - took me a while to figure out that the pMatrix of the object and the "copied" object point to the same address. So now I'm passing all Matrix objects that I don't want deconstructed by reference (e.g. void test(Matrix& a)). But now I'm getting the problem I spoke about earlier lol this is fun but I'm sure I'll start getting frustrated soon haha

Thanks,

Nick :)

[kbw]

I assume Matrix has member float** pMatrix.

pMatrix[i] = (float*)malloc(columns * sizeof(**pMatrix)); ought to be:
pMatrix[i] = (float*)malloc(columns * sizeof(*pMatrix));, but that shouldn't cause the program to crash.

I'd guess that you've gone off the end of you array while using Matrix elsewhere in the program.

[jsmith]

Also verify that you have implemented a proper copy constructor and assignment operator, as otherwise you
could be corrupting the heap.

[Disch]

*groans*

Don't use a 2D array for this. There's no point in doing all of this allocation. It just makes allocation/deletion code messier and adds all sorts of overhead.

Multidimentional arrays are evil.

Use a 1D array:

float* pMatrix;

//..

pMatrix = new float[ nRows * nCols ];  // only need one new/malloc

// .. when you need to access [x][y], do the following instead:

return pMatrix[ (y*nCols) + x ];

// .. and cleanup is much easier:

delete[] pMatrix;

*Disch stabs multidimentional arrays with the vorpal dagger firedraco gave him*

[NickPaul]

Haha yeah I agree with you Disch, that is a really nice way of doing this! I'll change it over once I figure out what is going on here lol might as well learn something from this.

Next, kbw - although I'm sure you're correct, I don't quiet understand it, I thought:

pMatrix = (float**)malloc(rows * sizeof(*pMatrix)); This creates a column of pointers, so I'm using the corresponding sizeof statement.

pMatrix[i] = (float*)malloc(columns * sizeof(**pMatrix)); Each pointer, created above, now points to the memory address malloc returns here which is the first element of the row. The row is an array of floats, so wouldn't I want to allocate memory for columns * sizeof(float)? - Anyway, I'll just say again, I'm sure you're correct lol I've been programming in C++ for like 1 week haha I just think I have an "error" in my understanding.

Lastly, jsmith haha you're going to love this! I haven't "implemented a proper copy constructor and assignment operator", I had no idea I had to do that lol it was all working fine before I implemented this destructor (apart from the massive memory leak lol). What kind of things should I be doing here? Any links you could provide me with?

Thanks to all that replied,

Nick :)

[NickPaul]

Haha I'll also ask this while I'm at it. Is it possible for me to stuff my computer up by running some of the programs I've been writing lately in C++ - like the one discussed in this post? Just wondering lol

[jsmith]

Your copy constructor and assignment operator both need to make a "deep" copy of
the pointer / matrix. As it stands, the default one provided by the compiler is only a
"shallow" copy (ie, it copies the pointer, not what the pointer points to).

class Foo {
  int* p;
 public:
    Foo( const Foo& f ) {
        p = new int;
        *p = *f.p;
   }
    // Assignment operator should do something very similar
};

[NickPaul]

Haha thanks jsmith! That will fix the problem I was having when passing the object to a function right? Since I was only passing a "shallow" copy when the function terminated it deleted the shallow copy which would run the decontructor and free the memory allocated to the pointer of both the original and the copy.

Thanks heaps,

Nick!

[NickPaul]

Yeah, jsmith that fixed the problem - runs again without freezing. Still got a memory leak - although the "flow rate" is much slower now. I was wondering if the following code could be a source of the problem:

    Matrix DCM_BE = funcL(1,phi)*funcL(2,theta)*funcL(3,psi);
    Matrix DCM_EB = funcL(3,-psi)*funcL(2,-theta)*funcL(1,-phi);
    Matrix DCM_AB = funcL(3,Va[2])*funcL(2,-Va[1]); 

.....

Matrix Matrix::operator * (Matrix m) const
{



    int rows1 = (*this).nRows;
    int cols1 = (*this).nCols;

    int rows2 = m.nRows;
    int cols2 = m.nCols;


    Matrix result(rows1,cols2);

    if( cols1 != rows2 ){

        cout << "ERROR: CANNOT MULTIPLY MATRICES";
        return result;

    }
    else{

        float tempResult;

        for (int i=0;i<rows1;i++)
        {

            for(int j=0;j<cols2;j++)
            {

                tempResult = 0;

                for(int k=0;k<rows2;k++)
                {
                    tempResult += pMatrix[i][k]*m.pMatrix[k][j];
                }

                result.pMatrix[i][j] = tempResult;

            }


        }



        return result;

    }


}

See funcL() returns a Matrix object and as you can see from above the multiplication operation returns a Matrix object. Will the deconstructor for the returned objects ever get called?

Thanks again,

Nick

[kbw]

You'll save yourself a Matrix copy with:
Matrix Matrix::operator * (const Matrix &m) const

but it really should be an external function:
Matrix operator*(const Matrix &a, const Matrix &b) const

The returned Matrix will be copied. So yes constructors and destructors will be called. It seems wastefull doesn't it?


On the other matter of your pointers. You want a 2D array of floats that you've chosen to implement as an array of an arrays of floats.

The inner array is an array of floats, float*.
The outer array is an array of an array of floats, float**.

So when you allocate the outer array, you want a float**.
And when you allocate the inner arrays, you want float*.

Got it?

It's kinder on the heap, cheaper to maintain, kinder to the CPU cache, and a bit simpler to use the method Disch proposed, a single block large enough to hold all the elements, then manually do the index calculations (rather than leaving it to the compiler).

[NickPaul]

Thanks kbw! That makes a lot more sense now.

Nick

[Disch]

Also note that you can overload the () operator to make lookup very easy with your matrix class:

class Matrix
{
...
  float& operator () (int x,int y)
  {
    return pMatrix[ (y*nCols) + x ];
  }
...
};

This way it's just as easy to look up elements in your matrix:

Matrix mymat( 4,4 ); // make a 4x4 matrix

// get element at x=2, y=1
float foo = mymat(2,1);