CODEWITHSUNDEEP
c++memory-leaks
jakeva
jakeva

Reputation: 2835

c++ delete reference

I'm still learning c++ and have a question that may be obvious, or maybe I just don't know what I'm trying to do. I have functions that take a matrix (a class I wrote, which has a properly written destructor) and create a new matrix from it, returning a reference to the new one. I need to iterate possibly tens of thousands of times on these matrices so I need to make sure I don't have any memory leaks. So, the question is, how do I properly delete the matrix I don't need any more in order to make space for the next one? Here's the code I'm trying to get leak-free:

DynamicMatrix<double> x0 = getX0(n);

DynamicMatrix<double>exactU = getExactU(n);

DynamicMatrix<double> b = getB(n) * w;

DynamicMatrix<double> x1 = getX1(x0, b, w, n);

while( !isConverged(exactU,x1,e) ){
    delete x0; //<<<<< This doesn't work. Nor does delete &x0.
    x0 = x1;
    x1 = getX1(x0, b, w, n);
}

Each of the getX() methods creates a pointer to a matrix, and returns a reference to the matrix as in getX0():

DynamicMatrix<double> &getX0(int n){
    DynamicMatrix<double>* mat1 = new DynamicMatrix<double>(n * n,1);
    for (int i = 1 ; i <= n; i++){
        for (int j = 1; j <= n; j++){
            mat1->set((i-1)*n +j, 1, 0);
        }
    }
    return *mat1;
}

So then, calling 'delete X0' errors because it needs a pointer. 'delete &X0' says the pointer being freed was not allocated. What is the correct way to do this? Or am I doing something completely wrong? With matrices too large and with too many iterations, my large hard drive runs out of space which I can only assume means I have memory leaks galore.

Upvotes: 4

Views: 3102

Answers (5)

Karl Knechtel
Karl Knechtel

Reputation: 61526

The rules for DynamicMatrix<double> are fundamentally the same as they are for int.

If it was allocated on the stack as an 'auto' variable, then the correct way to clean it up is to do nothing - just let it fall out of scope. You want to arrange your code, as much as possible, such that this is the case.

If it was allocated with 'new', clean it up with 'delete'.

Please don't ever dynamically allocate something and then return it by reference. Return the pointer. Actually, don't do that, either. Use a smart pointer class. Please.

Please don't dynamically allocate things if you don't need to. Just make a local value, and return it - by value (this is how you deal with the fact that you can't return a reference to a non-static local). You would never, ever, ever think about writing code like the following, right?

int& give_me_a_value() {
    int* result = new int(rand());
    return *result;
}

Again: the rules for DynamicMatrix<double> are fundamentally the same as they are for int. That's why you implement copy constructors, assignment operators and destructors: so that this actually works the way you'd reasonably expect it to.

Upvotes: 0

Sonny Saluja
Sonny Saluja

Reputation: 7287

Your bug is here:

DynamicMatrix<double> x0 = getX0(n);

You dont necessarily have to use pointers. You can return a reference to the newed object. To delete the memory just take the address of the reference.Taking the address of a reference gives you the address of the referent; you should be able to call 

// receive newed memory in a reference
DynamicMatrix<double>& x0 = getX0(n);

// &x0 should give you the address of the allocated memory.
delete &x0;

Upvotes: 0

Victor Nicollet
Victor Nicollet

Reputation: 24577

Stroustrup R'lyeh Fhtagn.

Writing MyType myVar = MyFunction() creates a brand new object using a constructor that accepts the return type of myFunction as an argument. Whatever was returned by myFunction is then discarded - in your example, getX0 returns a reference to an object that was allocated dynamically, and is therefore leaked.

Seriously, though - try creating the matrices on the stack (without new) and returning them as-is. Shouldn't cause too much trouble, since they appear to allocate their data dynamically on the inside anyway, and I suspect NRVO would apply to avoid making a copy (the returned matrix would be directly constructed into the appropriate location. The x0 and x1 magic at the bottom can be implemented as follows: 

x0.swap(x1);
DynamicMatrix<double> temp = getX1(x0, b, w, n);
x1.swap(temp);

Since a swap operation can be implemented on your dynamic matrix in terms of a pointer swap (which is very fast) instead of an actual data copy, this should be extremely fast.

Upvotes: 6

Keith Randall
Keith Randall

Reputation: 23265

You should use pointers. The statement

DynamicMatrix<double> x0 = getX0(n);

Makes a copy of a matrix. You want

DynamicMatrix<double> *getX0(int n){
  DynamicMatrix<double>* mat1 = new DynamicMatrix<double>(n * n,1);
  ...
  return mat1;
}

Then

DynamicMatrix<double> *x0 = getX0(n);
...
delete x0;

Upvotes: 2

Diego Sevilla
Diego Sevilla

Reputation: 29021

if getX() returns a pointer, you should write as the first line:

DynamicMatrix<double>* x0 = getX0(n);

That would make more sense, as you return a new pointer. Then you have to delete it as you show some lines below.

Note however that you can save a lot of troubles using boost::shared_ptr:

typedef boost::shared_ptr<DynamicMatrix<double> > dyn_matrix_ptr;

dyn_matrix_ptr x0 (getX0(n));
// use x0 as a normal pointer
...
// You don't have to manually delete it, it will be deleted automatically.

Upvotes: 1

Related Questions