Releasing memory for array of pointers?

Question

I have classes Deck and PlayingCard. A Deck object must have a dynamically allocated array of pointers to PlayingCard objects:

PlayingCard** _playing_cards;

To initialise this array, the Deck's constructor and build() functions are called:

Deck::Deck(int size)
{
    _total_playing_cards = size;
    _deal_next = 0;
    build();
}

void Deck::build()
{
    _playing_cards = new PlayingCard*[_total_playing_cards];
    for(int i = 1; i <= _total_playing_cards; ++i)
    {
        _playing_cards[i-1] = new PlayingCard(i % 13, i % 4);
    }
}

Releasing the memory allocated with 'new' is handled in the destructor:

Deck::~Deck()
{
    for(int i = 0; i < _total_playing_cards; ++i)
    {
        delete[] _playing_cards[i];
    }
    delete[] _playing_cards;
}

I then have a separate file, deck_test.cpp, which has a main() to simply construct and destruct a Deck object:

int main()
{
    Deck deck(52);
    deck.~Deck();
    return 0;
}

This compiles fine, but when debugging, Visual Studio reports "Unhandled exception at 0x5ab159da (msvcr100d.dll) in Playing Cards.exe: 0xC0000005: Access violation reading location 0xfeeefee2." When looking at the call stack, the problem appears to be occurring where I use the 'delete[]' operator in the 'for' loop within the destructor. Is this not the correct way to release memory from an array of pointers?

In the for loop, wouldn't a delete with no [] be sufficient, because you are deleting space for and object pointed to by _playing_cards[i]? — octopusgrabbus
– octopusgrabbus, Commented Feb 4, 2012 at 19:39

Timo Geusch · Accepted Answer · 2012-02-04 19:44:30Z

2

Your Deck destructor needs to read as follows:

Deck::~Deck()
{
    for(int i = 0; i < _total_playing_cards; ++i)
    {
        delete _playing_cards[i];
    }
    delete[] _playing_cards;
}

Note that in the loop, you have to use a non-array delete to delete a single playing card.

There is also another much bigger issue, namely that your are calling the destructor twice - once in your explicit call and the second time when deck goes out of scope at the end of main(). Basically you're never supposed to call a destructor on a non-heap allocated object manually in C++ as you're interfering with the built in lifetime management of C++ objects. Bad idea unless you (a) really know what you're doing and (b) you're doing it in very specific circumstances.

As an aside, using a dynamically allocated array of pointers with all the overhead that this brings is a bad idea unless you're learning about pointers and experimenting with them. In production code, please do yourself and everybody else a favour and use a std::vector instead.

answered Feb 4, 2012 at 19:44

Timo Geusch

24.4k5 gold badges55 silver badges71 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Ryan Loughborough Over a year ago

Agreed about the use of vectors, but it's an academic project, so my hands are sadly tied as to what I can and can't use.

Timo Geusch Over a year ago

Hence the comment "unless you're learning about pointers" :). In my opinion it's a good idea to learn about them, but there are far few legitimate uses of them in modern C++ code than people have you believe.

Sergey Vyacheslavovich Brunov · Accepted Answer · 2012-02-04 19:45:49Z

1

Please, do not call destructor directly in main().

Slightly modify the destructor code:

Deck::~Deck()
{
    if (_playing_cards) {
        for (std::size_t i = 0; i < _total_playing_cards; ++i) {
            delete _playing_cards[i];
            _playing_cards[i] = NULL;
        }
        delete[] _playing_cards;
        _playing_cards = NULL;
    }
}

By the way, why not use std::vector<PlayingCard>?

edited Feb 4, 2012 at 19:45

answered Feb 4, 2012 at 19:39

Sergey Vyacheslavovich Brunov

18.2k7 gold badges53 silver badges85 bronze badges

2 Comments

Seth Carnegie Over a year ago

That won't help, you'd just be dereferencing NULL on the second time through the destructor.

Seth Carnegie Over a year ago

You also don't need to set _playing_cards[i] to NULL.

Seagull · Accepted Answer · 2012-02-04 19:49:28Z

0

You don't have to call deck.~Deck(); by yourself. It will be called automatically. Just use:

int main()
{
    Deck deck(52);
    return 0;
}

And use the delete _playing_cards[i]; in the for loop, as delete[] means to delete an array and delete means to delete only one element.

answered Feb 4, 2012 at 19:49

Seagull

3,6782 gold badges34 silver badges44 bronze badges

Comments

Seth Carnegie · Accepted Answer · 2012-02-04 19:50:32Z

0

It's because of two things:

you call the destructor manually, and then the destructor is called again when the variable goes out of scope. You usually shouldn't call a destructor manually unless the object was allocated with placement new, or you sneakily reconstruct the object with placement new just before it goes out of scope (but don't do that).
delete[] _playing_cards[i]; should be delete _playing_cards[i] because playing_cards[i] is not an array, it's just a new PlayingCard.

Also, why are you doing i = 1; i <= _total_playing_cards in one place and i = 0; i < _total_playing_cards in another? It needlessly complicates things, I would advise picking one (preferably the latter) and sticking with it.

edited Feb 4, 2012 at 19:50

answered Feb 4, 2012 at 19:38

Seth Carnegie

75.3k22 gold badges185 silver badges252 bronze badges

Collectives™ on Stack Overflow

Releasing memory for array of pointers?

4 Answers 4

2 Comments

2 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

2 Comments

2 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related