Reputation: 382
I am working on a connect-four game simulator in C.
https://en.wikipedia.org/wiki/Connect_Four
The first step is to create a board environment for the game. I went ahead and made a data type board_t which is a struct that includes a dynamically sized array that will save moves played in a single dimension array. Board_t also includes height and width information of the board, so things can be retrieved in a correct manner.
I initialize this board in board_create() function, and use this initialized board_t variable in a board_can_play() function to check whether any play is possible in a given play. Here is the code.
#include <stdlib.h>
#include <assert.h>
#define PLAYER_BLUE 2
#define PLAYER_YELLOW 1
#define PLAYER_EMPTY 0
typedef unsigned char player_t;
typedef struct board_t
{
unsigned int width;
unsigned int height;
unsigned int run;
player_t * moves;
} board_t;
bool board_create (board_t ** b, unsigned int height, unsigned int width, unsigned int run, const player_t * i)
{
//Declare a board_t variable temp_b where parameters will be saved.
board_t temp_b;
//Create a pointer and malloc a memory location based on width and height.
temp_b.moves = malloc(sizeof(unsigned char)*(height*width));
//Itereate through the moves and initialize with the given player_t
int j;
for (j = 0; j < width*height; j++)
{
temp_b.moves[j] = PLAYER_EMPTY;
}
//Input all the values to temp_b
temp_b.height = height;
temp_b.width = width;
temp_b.run = run;
//Make a temporary pointer and assign that pointer to *b.
board_t * temp_b_ptr = malloc(sizeof(board_t));
temp_b_ptr = &temp_b;
*b = temp_b_ptr;
return true;
};
/// Return true if the specified player can make a move on the
/// board
bool board_can_play (const board_t * b, player_t p)
{
unsigned int i;
unsigned int height = board_get_height(b);
unsigned int width = board_get_width(b);
for(i = (height-1)*width; i < height*width; i++)
{
if (b->moves[i] == PLAYER_EMPTY)
{
return true;
}
}
return false;
}
However, whenever I call the board_t *b from board_can_play(), the program gives segmentation fault. More specifically,
if (b->moves[i] == PLAYER_EMPTY)
This line is giving me a segmentation fault. Also, functions that worked well in main(), is not working here in board_can_play(). For instance,
unsigned int height = board_get_height(b);
unsigned int width = board_get_width(b);
Are supposed to get 3 and 3, but getting 2 and 419678? I spent about 7 hours now figuring out, but cannot figure out what is going on.
Upvotes: 0
Views: 103
Reputation: 2518
I would approach you problem in the following way. Not that I have stubbed-in some error handling, as well as adding a method to destroy the board when done.
The following code compiles without warning in Ubuntu 14.01 LTS, using gcc-4.8.2. I compile the code with the following command line:
gcc -g -std=c99 -pedantic -Wall connect4.c -o connect4
Now, on to the code. You didn't provide a main, so I created a quick stub main:
#include <stdlib.h>
#include <stdbool.h>
#include <stdio.h>
#include <assert.h>
#define PLAYER_BLUE 2
#define PLAYER_YELLOW 1
#define PLAYER_EMPTY 0
typedef unsigned char player_t;
typedef struct board_t
{
unsigned int width;
unsigned int height;
unsigned int run;
player_t * moves;
} board_t;
bool board_create(board_t** b, unsigned int height, unsigned int width);
void board_destroy(board_t** b);
int board_get_height(const board_t* b);
int board_get_width(const board_t* b);
int main(int argc, char** argv)
{
board_t* pBoard = NULL;
if(board_create(&pBoard, 4, 4))
{
printf("board dimensions: %d by %d\n", board_get_height(pBoard), board_get_width(pBoard));
// TODO : put game logic here...
board_destroy(&pBoard);
}
else
{
fprintf(stderr, "failed to initialize the board structure\n");
}
return 0;
}
Not a lot to see in main, much like you would expect. Next is the board_create
function. Note that I deleted the run
and the player_t
parameters because i didn't see you use them in your code.
bool board_create(board_t** b, unsigned int height, unsigned int width)
{
bool bRet = false;
if(*b != NULL) // we already have a board struct laying about
{
board_destroy(b);
}
if(NULL != (*b = malloc(sizeof(board_t))))
{
(*b)->width = width;
(*b)->height = height;
if(NULL != ((*b)->moves = malloc(sizeof(unsigned char*)*(height * width))))
{
for(int j = 0; j < height * width; j++)
(*b)->moves[j] = PLAYER_EMPTY;
bRet = true;
}
else
{
/* TODO : handle allocation error of moves array */
}
}
else
{
/* TODO : handle allocation error of board struct */
}
return bRet;
}
Couple of comments on this function;
malloc
is check to make sure that we actually got the memory that we wanted. I tend to place the check in the same statement as the malloc
, but that is personal preference.true
regardless if all the allocations succeeded or not. Notice, that I only return true after both allocations are performed, and they succeeded.Ok, on the the new function I added, board_destroy
:
void board_destroy(board_t** b)
{
if(*b != NULL) // no board struct, nothing to do..
{
if((*b)->moves != NULL)
{
free((*b)->moves);
}
free(*b);
*b = NULL;
}
}
Some comments on this function;
free
that array first. (free
-ing the board structure first would mean that you lost your only reference to the moves array, and you would be leaking memory then).free
-ing the moves array, I again check to see that it exists. You didn't provide implementation details of board_get_* functions, but from their usage, I suspect that you have them implemented as:
int board_get_height(const board_t* b)
{
return (b->height);
}
int board_get_width(const board_t* b)
{
return (b->width);
}
I didn't do anything with your board_can_more
function due to not being sure how you intend to use it.
A quick run of the above code:
******@ubuntu:~/junk$ ./connect4
board dimensions: 4 by 4
******@ubuntu:~/junk$
My personal opinion is that when doing lots of memory allocations, frees in C or C++ you should run your program under valgrind periodically to make sure you are not leaking memory or have other memory related errors. Below is a sample of running this code under valgrind:
*****@ubuntu:~/junk$ valgrind --tool=memcheck --leak-check=full ./connect4
==4265== Memcheck, a memory error detector
==4265== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==4265== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright info
==4265== Command: ./connect4
==4265==
board dimensions: 4 by 4
==4265==
==4265== HEAP SUMMARY:
==4265== in use at exit: 0 bytes in 0 blocks
==4265== total heap usage: 2 allocs, 2 frees, 152 bytes allocated
==4265==
==4265== All heap blocks were freed -- no leaks are possible
==4265==
==4265== For counts of detected and suppressed errors, rerun with: -v
==4265== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Hope this helps, T.
Upvotes: 1
Reputation: 121387
In the if
statement that gives you segfault,
if (b->moves[i] == PLAYER_EMPTY)
The problem is not how moves
was allocated, but how b
itself was allocated. In board_create()
, you are returning a temporary object in here:
board_t * temp_b_ptr = malloc(sizeof(board_t));
temp_b_ptr = &temp_b;
*b = temp_b_ptr;
The malloc
'ed pointer is lost (you are overwriting it) and simply returning (through *b
) a pointer to a local variable.
So the move the allocation to the top and use temp_b_ptr
instead of temp_b
:
board_t *temp_b_ptr = malloc(sizeof(board_t));
if( !temp_b_ptr ) {
/* error handling */
}
....
....
*b = temp_b_ptr;
Upvotes: 2