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();
c++ memory issue
Author
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:
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
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.
You could just do this:
Foo Foo::doSomething()
{
Foo result;
// Do stuff to result.
return result;
}
Foo Foo::doSomething()
{
Foo result;
// Do stuff to result.
return result;
}
Author
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:
Edit: It actually looks like the assignment operator is not being called at all... let me look at this a bit more.
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.
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:
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;}
Your copy assignment operator isn't copying. It should copy into itself and then return a reference to self, i.e.:
Alternatively, you could just use a std::vector and let the compiler do the rest of the work for you:
Enigma
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
Author
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.
Thanks again.
Quote:Original post by Enigma
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
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();};This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
