Dynamic memory allocation in a class

[chakaflokiles]

Hello, I'm having problems with this code:

#include <iostream>

using namespace std;

class Foo
{
	public:
		Foo( int n );	// Constructor
		~Foo();		// Destructor
		int *ptr;
		int N;
};

Foo::Foo( int n )
{
	N   = n;
	ptr = new int[n];
	for ( int c = 0 ; c < N ; c++ )
		ptr[c] = c;
}

Foo::~Foo()
{
	N = 0;
	delete [] ptr;
}

void print ( Foo f )
{
	for ( int c = 0 ; c < f.N ; c++ )
		cout<<f.ptr[c]<<endl;
}


int main ( void )
{

	Foo a(5);
	print(a);
	system("pause");
	return 0;
}

I'm using Visual C++ 2008 version. The problem arises at the end, after the sentence 'system("pause")' is reached, which makes me think that the problem happens when calling the destructor. The destructor is called twice, the first time it's called is in the function print. The problem seems to be that the destructor can only be called once.
I know I can avoid this situation by defining the function print like this:

void print ( const Foo &f )
...

but I would like to know if there is some way I can do this keeping the definition that I've provided.

[ne555]

Your copy constructor is simply copying the pointers, so main::a.ptr and print::f.ptr point to the same place.
When those objects are destructed, you'll try to delete[] ptr twice.

You need to code a proper copy constructor that would handle that problem. There are several alternatives:
- the copy constructor does not exist. Copying is forbidden.
- you allocate another array an copy all the objects.
- both pointers point to the same array, but just one object may perform the delete. (ownership, or reference counting)

The same applies to the assignment operator.


The easiest way would be typedef std::vector<int> Foo;

[Disch]

There is a rule known as the "rule of 3". It says that if you need to write a copy constructor, assignment operator, or destructor.... you likely need to write all 3 of them.

In your case... when your Foo object is being copied, you are just copying the pointer. So what's happening is that 'a' in main is being copied to 'f' in print.

So a.ptr and f.ptr now point to the same memory.

When 'f' is destroyed, it will delete f.ptr (which also deletes a.ptr since they point to the same memory)

Then, when 'a' is destroyed, it attempts to delete that again resulting in the crash.


So fix, you need to write a copy constructor which will reallocate this buffer for the new object and do a deep copy:

Foo::Foo(const Foo& obj) // copy constructor
{
    // create a new buffer and copy over its contents
    N   = obj.N;
    ptr = new int[N];
    for ( int c = 0 ; c < N ; c++ )
        ptr[c] = obj.ptr[c];
}

You'll also need to write an assignment operator, or else you'll have the same issue when you try to assign objects.



Of course... none of this is an issue if you use smart pointers instead of manual memory management. My general rule is that if you are manually deleting... you're doing it wrong.


EDIT: ninja'd!