Sign in to follow this  

c++ memory issue

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

If you intended to correct an error in the post then please contact us.

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 this post


Link to post
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 this post


Link to post
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[i] = copy.array[i];
}
~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 this
Foo 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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
Share on other sites
Guest Anonymous Poster
better
Foo& Foo:operator=(const Foo & foo ){
if( this != &foo ){
//copy
}
return *this;
};

Share this post


Link to post
Share on other sites
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();
};

Share this post


Link to post
Share on other sites

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

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this