destructor problem

Question

this is my addCard function which takes a playingCard as a parameter then hands over the address of its self to an allocated array of pointers to playingCard objects.

void cardHand::addCard(playingCard card) {
    theHand[nElems++] = &card;
} // addCard()

now when i run my program it runs fine but then crashes when the destructor is called.

cardHand::~cardHand() {
    for(int c = 0;c<MAX;c++) {
     if(theHand[c] != NULL)
      delete theHand[c]; // here is the problem
    }
    delete [] theHand;
} // class destructor

is it crashing because I'm only handing over the address of the playingCard object in the addCard function. should it be a pointer instead?

Answer 1

It crashes on delete because it was never allocated with new.

void cardHand::addCard(playingCard card) {
    theHand[nElems++] = &card;
} // addCard()

In this function call, you are passing a temporary copy of the playingCard and taking its address. Storing addresses of temporaries is a no-no unless you really know what you're doing.

Instead, change that to something like

/** @param card card to add. Takes ownership. */
void cardHand::addCard(playingCard *card) {
    theHand[nElems++] = card;
} // addCard()

And in the calling code, pass it a playingCard object pointer returned from new playingCard.

It still has the issue of object ownership transfer which a common reason for double delete bugs or memory leaks. It's a good habit to make such transfers explicit with code comments.

Answer 2

The problem is here

void cardHand::addCard(playingCard card) { theHand[nElems++] = &card; }

You store address of the temporary card object which will be destructed at the end of the addCard method.

And in your destructor you attempt to delete it again.

You have two options.

First: make addCard to accept just card configuration and create your card with new in the addCard method.
Second: accept card by pointer but then your cardHand's destructor can't be responsible for card deletion. And deletion will perform Deck object which created all the cards.

Answer 3

When you say:

theHand[nElems++] = &card;

You are storing the address of a function parameter, which is effectively a local variable. This is always a bad thing to do, and in your case causes the crash when you attempt to delete it.

You probably want something like:

theHand[nElems++] = new playingcCard( card );

but the real solution is to use a std::vector of playingCard and to do away with dynamic allocation altogether.

Answer 4

So given the other answers, my way of doing it would be hand over a reference or a pointer to a playingCard.

And with regards to delete, the general rule is, you only delete what you newed, i.e. the one who allocates the memory should be responsible for its disposale. Of course, as with any rule, there are exceptions to this, but this behaviour needs to be documented very well in an interface's contract.

Answer 5

Rule of thump: Only delete what you allocated (either with new or with memalloc)

Therefore, your crash.

Answer 6

The reason it's not working is that you're passing your class by value in cardHand::addCard.
So what the compiler does is constructs a temporary copy of your class instance on the stack.
As it's on the stack, it will get automatically cleaned up once the addCard function returns.
You can fix this by passing your playingCard instance as a pointer instead.
However, as others in this post have said, it's advisable not to delete stuff that you didn't explicitly new.

Answer 7

You're using C++ as a better C, and although this is fine for many purposes it isn't idiomatic C++.

What you really want is to do away with dynamic allocation altogether. In general if you're writing C++ you should very rarely use new and even more rarely use delete.

Your hand should be declared something like this:

std::vector< playingCard > hand;

New cards should be put on it with something like this:

hand.push_back( card );

You should find that by using the C++ library's collection classes (and the TR1 smart pointes) you never need to use new or delete -- until you get to writing your own smart pointers anyway.