Jump to content
  • Advertisement
Sign in to follow this  
Nuc12

C++ properly releasing heap variables

This topic is 4412 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

The book I'm currently reading discusses how to implement a copy constructor or overload the = operator to return objects that contain member variables pointing to something on the heap. My question is, how dangerous can this be? Let's say we have the following:
Cat::Cat(const Cat & rhs){
  itsAge = new int;              //itsAge and weight are member variables but
  itsWeight = new int;           //they are pointers to ints
  *itsAge = 5;
  *itsWeight = 10;
}

int main(){
  Cat a = Cat(5,10);  // make a cat that is 5 years old and 10 pounds
  Cat b = Cat(3,5);
  a = b;
}


Does this cause a memory leak? What happens to the memory a was originally pointing to? The author deletes this memory in the destructor, but I don't see any reason why the destructor would be called here on the original object (a). Does the following code do the same thing?

Cat & Cat::operator=(const Cat & rhs){
  *itsAge = rhs.GetAge();
  *itsWeight = rhs.GetWeight();
  return *this;
}


If I assigned an object to another object (one that already existed and had actual member variables) would I be causing another memory leak? EDIT: Ah, I see the second code block doesn't call the new operator, so this definetly should not be a memory leak. TIA

Share this post


Link to post
Share on other sites
Advertisement
If you didnt have your custom assignment operator, the default assignment operator would be used.
If im not very wrong the default assignment operator would do something like this:

Cat & Cat::operator=(const Cat & rhs){
itsAge = rhs.itsAge;
itsWeight = rhs.itsWeight;
return *this;
}


Since itsAge and itsWeight is pointers to dynamically allocated memory, this would result in a memory leak as soon as you hit the statement a = b

However, the operator = you have provided solve this memory leak problem by assigning the values pointed to (not the pointers themself), so the pointers still point to the same heap memory as before the operator was called. Hence, no memory leak.

Share this post


Link to post
Share on other sites
Also, bear in mind that if you change your code slightly to:


Cat a=Cat(10,20);
Cat b=a;


then the copy constructor, not the assignment operator will be called. The default copy constructor will have the same problems pulpfist outlines about the default assignment operator.

Since you can't guarantee someone else won't use your code in this way, you should provide a copy constructor as well:


Cat::Cat(const Cat &C)
{
Age=new int; Age=*(C.Age); // etc
}


I appreciate that this is example code you are reading to illustrate the point but there is less and less need for this kind of dynamic allocation in C++ these days.

Share this post


Link to post
Share on other sites
The language can't read you mind WRT the "ownership" of memory. Therefore, the default does the simplest possible thing: it makes no assumption, and treats pointers just like any other data - and copies the *actual pointer, not the pointed-at data*. So by default, Cat b would end up pointing to the same age- and weight-representing ints.

That's bad for at least three reasons:

1) The memory leak, as noted. Now *noone* points at Cat a's *old* age- and weight-representing ints, because there are two sets of pointers and they both point at the other dynamic allocations. Now those old ints are leaked, because there's no way to get pointers back to them.

Notice this leak is nothing to do with allocating more memory; you can also leak memory by "forgetting" memory that was *already* allocated.

2) Pointers exist, in part, so that allocations can be "shared"; but here we don't *want* to do that. Now, the Cats 'a' and 'b' are tied together via the shared memory. If 'a' gains weight, so does 'b'. That doesn't really make any sense.

3) When one of the cats is destructed, it has no way of knowing that the other is holding on to the memory as well. If you always assume another Cat is pointing at the memory, then you can never delete any of it, and you leak more memory. If you assume no other Cats are pointing at it (i.e. that the Cat being destructed has a unique hold on the memory, and thus you delete it), then whenever that assumption is violated, everything goes kaboom when the *other* cat is destructed, because it will use the same destructor code and try to delete the memory again - boom.

As for "why a destructor would be called", it's called because the objects in question reach the end of their lifetime - either by a dynamically allocated Cat being deleted, or a local Cat going out of scope. In your example, both 'a' and 'b' are destructed at the end of main(). This is a good thing - but you have to be aware of it.

The provided assignment operator avoids the problem by adding the desired indirection to the assignment process, i.e. assigning the pointed-at int values rather than the pointer values.

Note that it's pointless to use the accessor like this. Objects *of the same type* get access to 'private' data, not just the current object. Meanwhile, if you later used the accessor for what it's good for - i.e. replacing the data members with some other representation, and hiding that fact from outside users - then you'd copy inaccurately. (Copying is an implementation detail that has to think in terms of the data members you have, not the abstraction you are trying to expose.)

There are better ways to take care of this stuff, of course. Try to make use of standard library containers like std::vector; for single objects, you can sometimes get away with std::auto_ptr, but that's a bad idea as a data member (it doesn't have the copy semantics you want for that purpose) - make your own wrapper instead. For implementing assignments safely and avoiding repetition of your work, there is the "copy and swap idiom".

You should also read up on the "Pimpl idiom" for an idea about how to avoid doing the work (and creating the indirection) for each individual allocated member (hint: put them in a struct and just dynamically allocate a struct instance), and also how you can make use of this kind of "memory-managed allocation" to hide more information from clients (basically, you can collapse the entire "private" section of the class in the header into a pointer-to-struct, and then you only change the header and do a full recompile when the *public* interface changes - which should be much rarer). This is generally considered more advanced stuff, which is a bit unfortunate.

Share this post


Link to post
Share on other sites
If your constructor allocates memory or resources, you must
A> Write a destructor and deal with that memory/resource
B> Either disable BOTH of your copy constructor and your operator=, or
C> Write a copy constructor and operator= that deals with the memory or resource

Writing that copy constructor/operator= is tricky.

Some options:
1> Your class could use "value-like" semantics. In that case, on a copy construction, you will want to allocate new resources/memory and then COPY the data from the source object. In operator=, you can either change the pointed to data, or delete the old data and create new storage for the new data.

2> Your class could use "pointer-like" semantics. In that case, you need to do some serious mojo for your resource/memory lifetimes. Reference counting is the easiest (but most fragile).

3> Your class could use "reference-like" semantics. This means you copy the pointers to the resources in your copy constructor, and modify the pointed to data in operator=. This makes your destructors very complex -- you now need to reference count or otherwise manage the lifetime of your data.


Tricky eh?

In general, an easy way to deal with the problem is to DISABLE copy/operator=.

If you need copy constructors/operator=, carefully write your copy constructor. Now write a "swap" class. Then, use the copy/swap idiom for operator=:

foo& operator=(foo const& other) {
foo tmp = other; // copy constructor
this->swap(tmp); // swap your data and tmp's data.
return *this; // tmp's destructor gets called, cleaning up this's old resources/memory
}


swap is easier to write than operator=. :)

Share this post


Link to post
Share on other sites
Alright so my first problem was thinking that the original "a=b" was calling the copy constructor instead of the overloaded "=" function. I was trying to take 2 separate programs from my book and combine them into one, when in fact my first example wouldn't call the copy constructor unless I had said Cat a(b) in which case there would be no issue with leaks in my copy constructor because a is a brand new object.

Alright so I get what Pulpfist is saying perfectly, but why then in EasilyConfused's example is the copy operator called and not the overloaded = operator function?

Zahlman, I see what you're saying about accessor methods but this isn't my code and the author of this book specifically mentioned that "it's a good programming practice to use public accessor methods when possible even though any Cat object can access private member data from any other Cat object". I do see your point though, and when I'm writing something meaningful I'll heed your advice :D

Quote:
if you later used the accessor for what it's good for - i.e. replacing the data members with some other representation, and hiding that fact from outside users - then you'd copy inaccurately.


This really confuses me. Why would I be using accessor methods inside my copy constructor (in a way that is different than how it was in the original code) or overloaded = operator to change anything? And how would that cause problems?

NotAYakk, thanks for the advice. It's going to take some time for me to digest what you're saying since I'm pretty new at this (as if you couldn't guess that).

Thanks for all the help!

Share this post


Link to post
Share on other sites
Quote:
Original post by Nuc12
but... the author of this book specifically mentioned that "it's a good programming practice to use public accessor methods when possible even though any Cat object can access private member data from any other Cat object".


!

I'm going to give the other 'experts' a fair chance to try to convince me of that, but that sounds like pretty bad advice to me. (Especially since accessors are overused in general.)

Share this post


Link to post
Share on other sites
Quote:
Original post by Nuc12
Alright so I get what Pulpfist is saying perfectly, but why then in EasilyConfused's example is the copy operator called and not the overloaded = operator function?


Because the fact that there is an equals sign in the statement in the expression is irrelevant.

Cat A=B;

is equivalent to:

Cat A(B);

since the statement is declaring the new Cat A. In the same way as when you do:

std::string s="hello";

it calls the std::string::(const char *) constructor.

With regard to the idea of using public accessor methods in a copy constructor as discussed above, I suppose you could argue that your public accessors are already generally doing some validation work and if you assign to values directly in the copy constructor you need to duplicate the validation, but I'm not 100% convinced of that either. This presupposes that public accessors are needed in the first place which, in a well designed system, is not necessarily the case.

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
Quote:
Original post by Nuc12
but... the author of this book specifically mentioned that "it's a good programming practice to use public accessor methods when possible even though any Cat object can access private member data from any other Cat object".


!

I'm going to give the other 'experts' a fair chance to try to convince me of that, but that sounds like pretty bad advice to me. (Especially since accessors are overused in general.)


I'd have to agree, if you find that you wrote a Set and a Get for a variable and they dont do anything special (i.e. check age isnt negative or check pointer isnt NULL (in which case you should swap to references)) then it should generaly be public (but not always, you know exceptions to every rule except those the compiler made thing)

Share this post


Link to post
Share on other sites
I guess you could also argue that if you went from (strawman alert)


class person
{
private:
int age,weight,height;

public:
void SetAge(int a){ age=a; }
void SetWeight(int w){ weight=w; }
void SetHeight(int h){ height=h; }
};


to


class person
{
private:
int attr[3];

public:
void SetAge(int a){ attr[0]=a; }
void SetWeight(int w){ attr[1]=w; }
void SetHeight(int h){ attr[2]=h; }
};


then by using the accessors in other member functions you are insulating the rest of the class from such representation changes.

I am purely devil's advocating here though. Obviously either of the above would be atrocious designs.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!