[machoolah]

Suppose that we have a class with a double pointer called element that is used in a method called operation. Operation does not change element and only uses it as an input. A double pointer is created in the main function and is assigned to the element property of an instance of the class.

I have two questions regarding the following code:
1. Is this a good practice? If not what is wrong with it?
2. Does the following code create a memory leak or dangling pointers? I am just deleting the double pointer created in the main function and set the element to NULL within the destructor of the class. (Since CLQ does not have the value for the size variable, I only set element = 0).

class CLQ{
public:
   int ** element;
   void operation(){
      // some statements
      // element is used as an input, does not change here.
   }
   ~CLQ{
      element = 0; 
   }

}

int main(){
   int size = 10;
   int ** e = new int *[size];  // e is a size-by-size dynamic array.
   for (int i = 0, i<size; i++){
      e[i] = new int [size]
   }

   CLQ myCLQ;
   CLQ.element = e;
   COQ.operation();

   for (int i = 0; i<size; i++){
       e[i] = 0;
       delete [] e[i];
   }
   e = 0;
   delete [] e;
   return 0;
}

[jjl]

1) It is bad because it bad OOP. One of the big ideas behind OOP is encapsulation.
2)It isn't a memory leak because you keep a handle on your memory with the double pointer you define in main.

This seems like a more appropriate approach for the problem

#include <iostream>

class CLQ {
public:
   static void operation(int **const element) {
	   //element pointer is constant, do what you need to do with it
   }
};

int main(){
   int size = 10;
   int ** e = new int *[size];
   for (int i = 0; i<size; i++) {
      e[i] = new int [size];
   }

   // pass 2d array to function
   CLQ::operation(e);

   for (int i = 0; i<size; i++){
       delete [] e[i];
   }
   delete [] e;
   return 0;
}

This post has been edited by jjl: 20 January 2012 - 05:06 PM

[baavgai]

Just because you can write memory leaks, doesn't mean you should. Your current code generates size+1 memory leaks. It's unclear how it could run without crashing.

Simply:

int *a = new int[10]; // allocated 10 ints to a
// do something 

a = 0;
// congratulations, a has now lost all reference to what it was pointing to.
delete [] a; // this should segfault you.  You deserve it.

It's also a profound OO fail. The class should contain all that nasty pointer stuff so the user needn't work about any possibility of a memory leak.
e.g.

#include <iostream>

class CLQ {
private:
	int **element; // no one messes with this, so no leak
	int size;
public:
	CLQ(int size);
	~CLQ(); // WTF? //element = 0; 
	int getSize() const { return size; }
	void operation() { }
};

int main(){
	CLQ myCLQ(10);
	
	myCLQ.operation();
	return 0;
}

CLQ::CLQ(int sz) : size(sz) {
	element = new int *[size];
	for (int i=0; i<size; i++){
		element[i] = new int [size];
	}
}

CLQ::~CLQ() {
	for (int i = 0; i<size; i++){
		// you DO NOT set to zero before delete
		// e[i] = 0;
		delete [] element[i];
		// actually, you don't set to zero at all...
	}
	delete [] element;
}

[blackcompe]

Ideally, you'd just put the 2d array in your class. You want to expose an easy-to-use client API. You don't want the client to handle the matrix, unless your class is a utility class, which would have mostly static methods, like in jjl's example. Here's an example. The client API is nice and easy to use:

IntMatrix mtrx(10, 10);
mtrx.set(0, 0, 10);
cout << mtrx.get(0, 0);
mtrx.invert();