Segmentation fault occurs when variable set to indexed parameter-CodePudding

I have a poker game where an array of the players' hands and an array of the cards in the middle are passed as arguments to a function. In the function get_winner, I can loop over and print the cards in the arrays (2nd and 3rd for loops), but if I set a variable to an element of an array and print it, I get a segmentation fault. Here is a simplified version of my code:

#include <iostream>

using std::cout, std::endl;

static const int num_players = 4;
static const char* SUITS[] = {"Hearts", "Diamonds", "Spades", "Clubs"};
static const char* VALUES[] = {"2", "3", "4", "5", "6", "7", "8", "9", "10", "Jack", "Queen", "King", "Ace"};

enum suit {hearts, diamonds, spades, clubs};
enum value {two, three, four, five, six, seven, eight, nine, ten, jack, queen, king, ace};

struct card 
{
    value v;
    suit s;
};

std::ostream& operator<<(std::ostream &out, const card &c)
{
    out << VALUES[c.v] << " of " << SUITS[c.s];
    return out;
}

int get_winner(const card (*hand)[num_players][2], const card (*commun_cards)[5])
{
    // this loop causes an error when printing the temp card variable  
    for (int i = 0; i < 5; i  ) {
         auto temp_card = commun_cards[i];
         cout << *temp_card << " " << (*temp_card).s << " " << (*temp_card).v << endl;
    }
    // these loops work fine and print the players' hands and community cards
    for (int i = 0; i < num_players; i  ) {
        cout << (*hand)[i][0] << ", " << (*hand)[i][1] << endl;
    }
    for (int i = 0; i < 5; i  ) {
        cout << (*commun_cards)[i] << endl;
    }
    return 0;
}

int main()
{
    card hands[num_players][2];
    card commun_cards[5];
    // fill hands and commun_cards with card structs
    hands[0][0] = card {jack, spades};
    hands[0][1] = card {nine, spades};
    hands[1][0] = card {ace, clubs};
    hands[1][1] = card {ace, hearts};
    hands[2][0] = card {three, diamonds};
    hands[2][1] = card {seven, spades};
    hands[3][0] = card {eight, diamonds};
    hands[3][1] = card {nine, clubs};
    commun_cards[0] = card {five, clubs};
    commun_cards[1] = card {king, hearts};
    commun_cards[2] = card {queen, spades};
    commun_cards[3] = card {two, spades};
    commun_cards[4] = card {ace, hearts};
    for (int i = 0; i < num_players; i  ) {
        cout << "Player " << i << " cards: " << hands[i][0] << ", " << hands[i][1] << endl;
    }
    cout << "\nCommunity cards:\n";
    for (int i = 0; i < 5; i  ) {
        cout << commun_cards[i] << endl;
    }
    get_winner(&hands, &commun_cards);
}

CodePudding user response：

This loop should be

// this loop causes an error when printing the temp card variable  
for (int i = 0; i < 5; i  ) {
     auto temp_card = (*commun_cards)   i; // change here
     cout << *temp_card << " "<< temp_card->s << " " << temp_card->v << endl;  // change here
}

auto temp_card = &(*commun_cards)[i]; is another way of writing auto temp_card = (*commun_cards) i; and temp_card->s is another way of writing (*temp_card).s.

But you've written code that is unnecessarily complex by passing pointers to your arrays instead of pointers to your array elements. The name of an array decays into a pointer to it's first element, so card commun_cards[5] becomes card *commun_cards and card hand[num_players][2] becomes card (*hand)[2]. This is how you normally pass an array to a function.

Here's the whole function simplified

int get_winner(const card (*hand)[2], const card *commun_cards)
{
    // this loop causes an error when printing the temp card variable  
    for (int i = 0; i < 5; i  ) {
        auto temp_card = commun_cards[i];
        cout << temp_card << " " << temp_card.s << " " << temp_card.v << endl;
    }
    // these loops work fine and print the players' hands and community cards
    for (int i = 0; i < num_players; i  ) {
        cout << hand[i][0] << ", " << hand[i][1] << endl;
    }
    for (int i = 0; i < 5; i  ) {
        cout << commun_cards[i] << endl;
    }
    return 0;
}

then

int main()
{
    ...
    get_winner(hands, commun_cards);
}

CodePudding user response：

Your function's 1st loop is accessing the commun_cards array incorrectly. Its 3rd loop is accessing the array correctly. Why would you expect the 1st loop to need to access the elements any differently than the 3rd loop just because it wants to save each element to a variable?

Since you are passing in each array by pointer, you must dereference each pointer before you can then index into the elements of each array. Your 2nd and 3rd loops are doing that. Your 1st loop is not.

Your 1st loop is expecting commun_cards[i] to yield a pointer to each element, but that is simply not true, which is why *temp_card then crashes. If you really want a pointer to each element, you need to use this instead:

auto *temp_card = &(*commun_cards)[i];
// alternatively:
// auto *temp_card = (*commun_cards)   i;
cout << *temp_card << " " << temp_card->s << " " << temp_card->v << endl;

Otherwise, use a reference to each element instead of a pointer:

auto &temp_card = (*commun_cards)[i];
// alternatively:
// auto &temp_card = *((*commun_cards)   i);
cout << temp_card << " " << temp_card.s << " " << temp_card.v << endl;

If you change the function to accept the arrays by reference instead of by pointer (which I highly suggest you do), then the function would become this:

int get_winner(const card (&hand)[num_players][2], const card (&commun_cards)[5])
{  
    for (int i = 0; i < 5; i  ) {
         auto &temp_card = commun_cards[i];
         cout << temp_card << " " << temp_card.s << " " << temp_card.v << endl;
    }
    for (int i = 0; i < num_players; i  ) {
        cout << hand[i][0] << ", " << hand[i][1] << endl;
    }
    for (int i = 0; i < 5; i  ) {
        cout << commun_cards[i] << endl;
    }
    return 0;
}

...

get_winner(hands, commun_cards);