C++: values in a class instance randomly gets garbage values

Question

I have been searching and debugging for days and I can't think of a reason this is happening as I have never experienced such a weird bug. This is a long post so I apologize in advance.

I am writing a poker game in c++ which contains 10+ classes so far, so I'll briefly describe the relevant members of the relevant classes, for reference. a Game consists of a Table and an std::vector of std::unique_pointers of Players (in practice the players are actually derived classes of Player). a Table consists of a Deck (a simple dynamic array of Cards). Every Card is just a value (int) and a symbol (enum). a Player has a Hand (2 Cards) and a pointer to the Table object the Game instance creates.

The game loop is a number of bots (2-10) are playing each other poker matches until they lose all their chips. every match the Table (m_table is reset), the deck is recreated and shuffled again.

the following code is a simplified version of the code which causes the error, but is exactly the same:

// Game.h

class Game {
    std::vector<std::unique_ptr<Player>> m_players;
public:
    Game();

    void gameLoop();
    void drawCards();
}

// Game.cpp:

Game::Game() : m_players() {
    m_players.reserve(10);
    for (int i=0;i<10;i++)
        m_players.push_back(std::make_unique<Player>());
}
void Game::gameLoop() {
    while (true) { // while true just for simplification
        drawCards();

        // do stuff
    }
}

void Game::drawCards() {
    Card one, two;

    for (auto &player : m_players) { // new cards for all the players
        one = m_table.takeCardFromDeck(); // <--- the line causing the error
        std::cout << one << " was drawn" << std::endl; // <--- access violation 
                                                       // here
        two = m_table.takeCardFromDeck();
        std::cout << two << " was drawn" << std::endl;

        // do stuff with the 2 cards
    }
}

// Table.h

class Table {
    Deck m_deck;
public:
    const Card takeCardFromDeck();
}

// Table.cpp:

const Card Table::takeCardFromDeck() {
    return m_deck.takeTopCard();
}

// Deck.h

#define MAX_SIZE 52

class Deck {
    Card *m_deck;
    int m_curAmount;
public:
    Deck();

    inline bool isEmpty() { return m_curAmount == 0; }
    void newDeck();
    const Card takeTopCard();
}
// Deck.cpp:

Deck::Deck() 
    : m_deck(new Card[MAX_SIZE]), m_curAmount(MAX_SIZE) {
    newDeck();
    // shuffle deck
}
void Deck::newDeck() {
    m_curAmount = MAX_SIZE; // 52
    int count = 0;
    for (int i = (int)Symbol::club; i < (int)Symbol::null; i++)
        for (int j = 1; j <= 13; j++) {
            m_deck[count] = Card(j, (Symbol)i);
            count++;
        }
}
const Card Deck::takeTopCard() {
    if (isEmpty())
        throw std::out_of_range("cant take card, deck empty");

    Card card = m_deck[m_curAmount - 1];
    m_deck[m_curAmount - 1] = Card();
    m_curAmount--;

    return card;
}

// Card.h

enum class Symbol {
    club, heart, spade, diamond, null
};

class Card {
    int m_value;
    Symbol m_symbol;
public:
    Card(int value, Symbol symbol);
    inline bool isNull() const { return m_symbol == Symbol::null || m_value == 0; 
    }
    const char *valueToString() const;
    const char *symbolToString() const;
    friend std::ostream &operator<<(std::ostream &output, const Card &source);
}

// Card.cpp:

Card::Card(int value, Symbol symbol) : m_value(value), m_symbol(symbol) {}
const char *Card::valueToString() const {
    if (this->isNull())
        return "nullptr";

    switch (m_value)
    {
    case 1:
        return "Ace";
    case 2:
        return "2";
    case 3:
        return "3";
    case 4:
        return "4";
    case 5:
        return "5";
    case 6:
        return "6";
    case 7:
        return "7";
    case 8:
        return "8";
    case 9:
        return "9";
    case 10:
        return "10";
    case 11:
        return "Jack";
    case 12:
        return "Queen";
    case 13:
        return "King";
    }
    return "null";
}
const char *Card::symbolToString() const {
    if (this->isNull())
        return "nullptr";

    switch (m_symbol) {
    case Symbol::club:
        return "club";
    case Symbol::heart:
        return "heart";
    case Symbol::spade:
        return "spade";
    case Symbol::diamond:
        return "diamond";
    }
    return "null";
}
std::ostream &operator<<(std::ostream &output, const Card &source) {
    if (source.isNull())
        output << "nullptr";
    else
        output << '(' << source.valueToString() << ',' << source.symbolToString() << ')';
    return output;
}

The error occurring is that randomly (it can happen in the first match played, the 100th or not happen at all) is that the first card that gets drawn from the table and put in to the first player's hand, is put in as garbage (as if the Card object is randomly destroyed somewhere from when it gets drawn from the deck up to when it is put in the one variable). when that card is being printed there is an access violation error and the card's values as I said are shown to be garbage. This seems like a data racing type of error but my program is synchronous, no use of async or threads.

I tried debugging, but I never managed to catch this in a debugging run so I don't know where the Card is destroyed: takeTopCard() method, takeCardFromDeck() method or Card's assignment operator.

If anyone is wondering I am using visual studio 2019. This bug is driving me nuts so any help would be very much appreciated.

SOLVED: Alright so I'm not sure why this fixed it (or why this was a bug in the first place), but I changed one and two to const references and it fixed the issue:

const Card &one = m_table.takeCardFromDeck();
const Card &two = m_table.takeCardFromDeck();

Anyone know why this bug was happening?

Answer 1

Does your original code (without the const & fix) have the same bug if you don’t print the “xxx was drawn” message?

void Game::drawCards() {
    Card one, two;

    for (auto &player : m_players) { // new cards for all the players
        one = m_table.takeCardFromDeck();
        std::cout << one << " was drawn" << std::endl;
        two = m_table.takeCardFromDeck();
        std::cout << two << " was drawn" << std::endl;

        // do stuff with the 2 cards
    }
}

I am not 100% sure about where the problem is, but it’s possible that, std::cout do not always convert one and two into strings immediately. Instead, their references are put into something like a buffer. When std::cout finally decided that it wants to flush the buffer, object references in the buffer are then converted into strings.

Since your original code uses local variable to storage the card drawn(as value, not reference). When flushing the buffer later on, sometimes c++ tries to access a references to a Card local variable, which is not there anymore after the function finished executing.

Changing the type of the local variable into references means that the object reference inside the buffer now points all the way back to the Card object in m_table and ultimately m_deck, instead of some local variables inside Game::drawCards(), so it works.

The inner working of std::cout depends highly on the implementation of the standard library that’s being used.

As crazy as it sounds, this is the only explanation I could come up with if all you did to fix the bug is to change two variables to reference type, and assuming other parts of your code, eg. Card::valueToString, doesn’t have any bugs.

One way to test this hypothesis is to (with the old code) remove the std::cout entirely, or explicitly convert one and two into strings before passing them into std::cout, and see if the bug remains.