CODEWITHSUNDEEP
c
yxting
yxting

Reputation: 51

Illegal operation when setting array values in struct

I have the following 3 structs for a card game program

// Linked list of cards, used for draw & discard pile, players' hands
typedef struct cardStack {
    struct cardStack *next;
    Card *card;
} CardStack;

// A player and their hand
typedef struct player {
    int playerNumber;
    CardStack *hand;
} Player;

typedef struct _game {
    CardStack *discardPile;
    CardStack *drawPile;    
    Player *players[4];
    int currentPlayer;
    int currentTurn;
} *Game;

and this function to initialise the game struct

Game newGame(int deckSize, value values[], color colors[], suit suits[]) {

    Game game = (Game)malloc(sizeof(Game));

    game->players[0] = (Player*)malloc(sizeof(Player));
    game->players[0] = &(Player){0, NULL};
    game->players[1] = (Player*)malloc(sizeof(Player));
    game->players[1] = &(Player){1, NULL};
    game->players[2] = (Player*)malloc(sizeof(Player));
    game->players[2] = &(Player){2, NULL};
    game->players[3] = (Player*)malloc(sizeof(Player));
    game->players[3] = &(Player){3, NULL};

    for (int i = 1; i <= 7; i++) {
        for (int j = 1; i <= NUM_PLAYERS; i++) {
            Card card = newCard(values[i * j - 1], colors[i * j - 1], suits[i * j - 1]);
            addToStack(card, game->players[j-1]->hand);
        }
    }

    CardStack *discardPile = (CardStack*)malloc(sizeof(CardStack));
    Card firstDiscard = newCard(values[28], colors[28], suits[28]);
    addToStack(firstDiscard, discardPile);
    game->discardPile = discardPile;

    CardStack *drawPile = (CardStack*)malloc(sizeof(CardStack));
    for (int i = 29; i < deckSize; i++) {
        Card card = newCard(values[i], colors[i], suits[i]);
        addToStack(card, drawPile);
    }
    game->drawPile = drawPile;

    game->currentPlayer = 0;
    game->currentTurn = 1;

    return game;
}

It compiles fine, but when I try to run it, this line 

game->players[0] = (Player*)malloc(sizeof(Player));

and similar, give an error "illegal array, pointer or other operation" I'm not sure whats wrong as I am just setting one pointer (in the array of pointers in the struct) to another

EDIT: unfortunately this is an assignment where the header file was given so I have no choice but to use the pointer typedef

Upvotes: 1

Views: 155

Answers (3)

Jonathan Leffler
Jonathan Leffler

Reputation: 753870

Quite apart from the memory allocation issue identified by John Kugelman, you have at least one more major problem…

Another problem

Note that the lines like these two:

game->players[0] = (Player*)malloc(sizeof(Player));
game->players[0] = &(Player){0, NULL};

carefully leak the allocated memory. You replace the just allocated pointer with a pointer to the compound literal, which leaves you no way to free the allocated memory. It is perfectly legitimate to modify the compound literal as long as it doesn't go out of scope — but it does go out of scope when the function returns, so you not only leak memory but you also modify data you no longer own if you ever change the player information.

You probably want this instead, which copies the compound literal to the allocated memory:

game->players[0] = (Player*)malloc(sizeof(Player));
*game->players[0] = (Player){0, NULL};

I wouldn't be surprised to find there are other issues lurking, but the code is not an MCVE (Minimal, Complete, Verifiable Example) so it is hard to be sure.

Upvotes: 2

John Kugelman
John Kugelman

Reputation: 361615

typedef struct _game {
    ...
} *Game;

Game is defined as an alias for struct _game *, a pointer.

Game game = (Game)malloc(sizeof(Game));

That means that sizeof(Game) is the size of a pointer and not the entire struct. A pointer is smaller than the entire struct, so it's not enough memory. Writing to ->players accesses memory outside of the malloc'ed area which causes the illegal operation error.

A correct allocation would be:

Game game = malloc(sizeof *game);

Lesson learned: use p = malloc(sizeof *p) rather than p = malloc(sizeof(Type)) to avoid this kind of mistake. The compiler won't catch a size mismatch. sizeof *p will always be the right size, even if p changes type.

And if possible, get rid of the * in the definition of Game! It's quite out of place.

Upvotes: 3

abc
abc

Reputation: 2027

Don't use pointers at the end of struct definitions it's ugly.

Change it to:

typedef struct _game {
   CardStack *discardPile;
   CardStack *drawPile;    
   Player *players[4];
   int currentPlayer;
   int currentTurn;
} Game;

And in your malloc replace that statement with the following:

Game* games = (Game*)malloc(sizeof(Game));

Bonn-appetite!

Upvotes: 0

Related Questions