Public Group

# c++ memory issue

This topic is 4669 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Being a self-taught programmer, I occasionaly come across strange gaps in my understanding of C++. This seems like it would be fairly critical, but I've never actually had this problem before now. Anyway, enough excuses... I have a class that is built like this:
class Foo
{
private:
float* array;
public:
Foo() { array = new float[144]; }
Foo(const Foo& copy) { // not so relevent };
~Foo() { delete[] array; array = 0; }

Foo doSomething();
};

Foo Foo::doSomething()
{
Foo* result = new Foo();

// Do stuff to result.
return *result;
}

// Then in the main program I do something like this
Foo temp;
temp = temp.doSomething();


Now right after that statement the destructor is called and deletes the new Foo I just created. How should the class (or, the doSomething function) look to prevent this error? Why is this even happening anyway? Thanks in advance, Fibon

##### Share on other sites
Well, the real error in that code is a memory leak. The Foo which is newed in doSomething is never deleted. The return statement invokes the copy constructor for the result and then the assignment operator fires for the assignment. (Or does it just fire the assignment operator.... I can never remember how that plays out.) Assuming you've implemented both of those correctly, those will end up sticking a Foo with a brand new array into temp, but will never destroy the originally newed Foo. To get around this, don't new it in doSomething, or use an auto_ptr to fire the destructor automatically after exiting the scope.

##### Share on other sites
You could just do this:

Foo Foo::doSomething()
{
Foo result;

// Do stuff to result.

return result;
}

##### Share on other sites
It looks like it is calling the copy constructor then the assignment operator.

All right, I can see how I slipped there and that there's no need to use new. However I still seem to have the same problem (temp's array has been deleted after calling temp = temp.doSomething();). I'd like to keep this code as minimalistic as possible, so I'd prefer to get this without smart pointers.

If you (or anyone else) don't mind helping a bit more, here's what I have now:

class Foo{private:   float* array;public:   Foo() { array = new float[144]; }   Foo(const Foo& copy)    {         array = new float[144];       for (int i = 0; i < 144; i++)            array = copy.array;   }   ~Foo() { delete[] array; array = 0; }   Foo operator=(const Foo& copy)   {       return Foo(copy);   }   Foo doSomething();};Foo Foo::doSomething(){    Foo result;    // Do stuff to result.    return result;}// Then in the main program I do something like thisFoo temp;temp = temp.doSomething();

Edit: It actually looks like the assignment operator is not being called at all... let me look at this a bit more.

##### Share on other sites
That code doesn't have any problems I can see, although it looks like it's probably doing a few more construction/destruction cycles than it needs to. That's endemic to C++, though.

EDIT: Oh, except that your assignment operator is teh weird. It should look like this:
Foo& Foo::operator=(const Foo& copy){    // copy "copy"'s contents into "*this"    return *this;}

##### Share on other sites
Your copy assignment operator isn't copying. It should copy into itself and then return a reference to self, i.e.:
Foo & operator=(Foo const & foo){	std::copy(foo.array, foo.array + 144, array);	return *this;}

Alternatively, you could just use a std::vector and let the compiler do the rest of the work for you:
class Foo{	private:		std::vector< float > array;	public:		Foo()			:			array(144)		{		}};

Enigma

##### Share on other sites
Whow, that was fast. Thanks for your help guys. I suspect knowing how to write a correct assignment operator will be a very useful thing to know =).

Thanks again.

##### Share on other sites
better
Foo& Foo:operator=(const Foo & foo ){
if( this != &foo ){
//copy
}
return *this;
};

##### Share on other sites
Good point. Actually, this whole page is a useful read.

##### Share on other sites
Quote:
 Original post by EnigmaYour copy assignment operator isn't copying. It should copy into itself and then return a reference to self, i.e.:Foo & operator=(Foo const & foo){ std::copy(foo.array, foo.array + 144, array); return *this;}Alternatively, you could just use a std::vector and let the compiler do the rest of the work for you:class Foo{ private: std::vector< float > array; public: Foo() : array(144) { }};Enigma

Alternatively, since the size is known and doesn't change over the object's lifetime (ignore this if you need to resize the array), just use an array member:
class Foo{private:   float array[144];public:   // If that's the only data, we don't need any ctor etc. at least for   // the functionality we know about so far :)   Foo doSomething();};

1. 1
Rutin
46
2. 2
3. 3
4. 4
5. 5
JoeJ
19

• 11
• 13
• 9
• 10
• 12
• ### Forum Statistics

• Total Topics
633003
• Total Posts
3009825
• ### Who's Online (See full list)

There are no registered users currently online

×