1

I'm a C# programmer having problems with messy pointers and I just can't find out what the mistake is.. I could use some help with the list

So basically I have something like a stack of cards and these cards are saved in a list. I just want to take the upper most and return it to the function. I could use pop_back() but the last card has to stay as it is because it is the cardback (I'm making it later with textures and stuff)

Card * CardStack::HandOut()
{
    if (m_Stack.size() > 1)
    {
        list<Card *>::iterator it = m_Stack.end();
        advance(it, -2);
        Card *ret = *it;
        Card tmp = *ret;
        Card *tmpp = &tmp;
        m_Stack.remove(ret);
        return tmpp;
    }
    return NULL;
}

So I want to always pop the second last Card back. I'm sure its a total beginner mistake :(

TheSentry
  • 23
  • 1
  • 4

2 Answers2

3

You are returning a pointer to a local variable,

Card tmp = *ret;
Card *tmpp = &tmp;
m_Stack.remove(ret);
return tmpp;

that doesn't exist anymore after the function exited. So when you use the pointer later, you invoke undefined behaviour.

You should not bother with tmp and tmpp, returning ret ought to do it, the remove doesn't destroy the card, it just removes (the pointer to) it from the stack.

Daniel Fischer
  • 181,706
  • 17
  • 308
  • 431
  • Yea I already thought that'll be a problem... So how could I copy the entire object since there is no .copy() of any kind as far as I know :( – TheSentry Oct 22 '12 at 18:57
  • You don't need to, `ret` already points to the card, and `remove` just removes it (the pointer to the card, actually) from the stack. – Daniel Fischer Oct 22 '12 at 19:00
  • Wow thanks right thats the thing I wasn't aware of ^^ I thought remove just deletes the entire object :D Appreciate the fast help! Thanks :) – TheSentry Oct 22 '12 at 19:00
  • Great! Everthing works Just removed Card tmp = *ret; Card *tmpp = &tmp; and return ret :D thanks again ;) – TheSentry Oct 22 '12 at 19:02
1

You could just erase the item pointer by the iterator directly. This also ensures the removal is O(1) instead of O(n) using .remove(), and avoids removing extra items if the contents are not unique.

    std::list<Card*>::iterator it = m_Stack.end();
    std::advance(it, -2);
    Card* res = *it;
    m_Stack.erase(it);
    return res;

Note that it is not idiomatic in C++ to store raw pointers. It is better to store the object by value (i.e. use list<Card>) if copying is cheap, or use a smart-pointer (e.g. list<shared_ptr<Card> >) so the memory can be collected automatically when it is no longer used.

kennytm
  • 510,854
  • 105
  • 1,084
  • 1,005
  • Interesting solution :) thanks I should get more into interators! – TheSentry Oct 22 '12 at 19:03
  • I'll have a look at shared pointers thanks :) and when I use shared ponters I don't have to worry about deleting them? – TheSentry Oct 22 '12 at 19:08
  • @TheSentry: Almost correct. shared_ptr uses reference-counting, so if you make a circular reference it won't be freed. But it's still much better than manually managing it. See also http://stackoverflow.com/questions/106508/what-is-a-smart-pointer-and-when-should-i-use-one. – kennytm Oct 22 '12 at 19:17
  • Thanks :) but now I changed my list to simply and I get a bad error from the remove function :/ – TheSentry Oct 22 '12 at 19:19
  • @TheSentry: That's likely because you didn't overload the `==` operator for `Card`. The .remove() function relies on equality comparison to remove the items. – kennytm Oct 22 '12 at 19:22
  • o.O why overload the == operator? I just want to remove the it from the stack with m_Stack.remove(it) which doesn't work anymore. Before I did it with the ret which was a Card* var. That worked :/ Now that I have changed over to simply Card it broke... – TheSentry Oct 22 '12 at 19:25
  • Ohhhhh now I get it :D But what do I have to write into the == operator? – TheSentry Oct 22 '12 at 19:36