Dynamic allocaiton of a 2D-array

[John den Haan]

Hello!

Again a silly pointer-problem! For a simple ASCII-game I am planning to
code, I wrote the following routine to allocate memory for the viewport:

--

bool allocate_viewport (world_square ***vis_squares, int rows, int cols)
{
int i;

*vis_squares = (world_square**) malloc(rows * sizeof(world_square*));

if (vis_squares == NULL) {
return false;
}
for (i=0; i<25; i++) {
*vis_squares = (world_square*) malloc(cols *
sizeof(world_square));
if (*vis_squares == NULL) {
return false;
}
}
return true;
}

--

Where world_square is the typedef of a struct containing some info on
the world tile (longs and ints basically). The goal is to be able to
access viewable tiles by simply using array notation as in
vis_squares[x][y]. In other words: I want to allocate memory for a
2D-array containing the viewable tiles.

This function is invoked in main.c in the following manner;

world_square **vis_squares;
if (!initialize()) {
printf("ERROR: Failed to initialize\n");
return 1;
}
allocate_viewport (&vis_squares,25,80);

But it (ofcourse) fails horribly: windows (XP pro) bails with a useless
error message. What am I doing wrong here? I am at loss. Running the app
through GDB doesn't help me much, except noting that it fails at i=2...

Thanks for your time,

John den Haan

 

[Alan Curry]

bool allocate_viewport (world_square ***vis_squares, int rows, int cols)
{
int i;

*vis_squares = (world_square**) malloc(rows * sizeof(world_square*));

if (vis_squares == NULL) {
return false;
}
for (i=0; i<25; i++) {
*vis_squares = (world_square*) malloc(cols *
sizeof(world_square));
if (*vis_squares == NULL) {

You meant (*vis_squares) but you wrote *vis_squares which actually
means *(vis_squares). Since vis_squares (the triple-pointer) only points
to a single double-pointer (the one declared in the caller), vis_squares
doesn't point to a valid object for any value of i other than 0. So you
corrupted some unrelated memory and the program crashed.

And shouldn't that 25 be rows?

return false;
}
}
return true;
}
[...]

world_square **vis_squares;
if (!initialize()) {
printf("ERROR: Failed to initialize\n");
return 1;
}
allocate_viewport (&vis_squares,25,80);

 

[John den Haan]

Alan Curry schreef:

You meant (*vis_squares) but you wrote *vis_squares which actually
means *(vis_squares). Since vis_squares (the triple-pointer) only points
to a single double-pointer (the one declared in the caller), vis_squares
doesn't point to a valid object for any value of i other than 0. So you
corrupted some unrelated memory and the program crashed.

Thank you! I really needed an extra pair of eyes on that one

And shouldn't that 25 be rows?

Yes it should be Thanks again for your sharpness.

- John

 

[Barry Schwarz]

Hello!

Again a silly pointer-problem! For a simple ASCII-game I am planning to
code, I wrote the following routine to allocate memory for the viewport:

--

bool allocate_viewport (world_square ***vis_squares, int rows, int cols)
{
int i;

*vis_squares = (world_square**) malloc(rows * sizeof(world_square*));

Make life easier on yourself:

Don't cast the return from malloc to a pointer type. It never
helps and could hide a diagnostic you would really want to see.

Let the operand of sizeof be variable being assigned preceded
by *, as in:
*vis_squares = malloc(rows * sizeof **vis_squares);

if (vis_squares == NULL) {

You probably meant *vis_squares.

return false;
}
for (i=0; i<25; i++) {

You want rows here, not 25.

*vis_squares = (world_square*) malloc(cols *
sizeof(world_square));

After fixing the precedence problem, this would become
(*vis_squares) = malloc(cols * sizeof *(*vis_squares);

if (*vis_squares == NULL) {

Another precedence problem.

return false;
}
}
return true;
}

Remove del for email

 

[Chris Torek]

bool allocate_viewport (world_square ***vis_squares, int rows, int cols)
{
int i;

*vis_squares = (world_square**) malloc(rows * sizeof(world_square*));

[/QUOTE]

Make life easier on yourself:

Don't cast the return from malloc to a pointer type. It never
helps and could hide a diagnostic you would really want to see.

Let the operand of sizeof be variable being assigned preceded
by *, as in:
*vis_squares = malloc(rows * sizeof **vis_squares);

Agreed. I would also add, however, that one can make this even
easier, by making a local "world_square **" pointer:

bool allocate_viewport(world_square ***resultp, int rows, int cols) {
bool failed = false;
world_square **squares;
int i;

squares = malloc(rows * sizeof *squares);
if (squares == NULL)
failed = true;
else {
for (i = 0; i < rows; i++) {
squares = malloc(cols * sizeof *squares);
if (squares == NULL) {
failed = true;
break;
}
}
if (failed) {
/* release the ones we successfully allocated */
while (--i >= 0)
free(squares);
/* and the array itself */
free(squares);
squares = NULL;
}
}
*resultp = squares;
return !failed;
}

One might also be able to improve this by doing just two malloc()s:

bool allocate_viewport(world_square ***resultp, int rows, int cols) {
world_square **squares;
int i;

/* allocate enough space for row-pointers */
squares = malloc(rows * sizeof *squares);
if (squares != NULL) {
/* allocate enough space for all rows*cols data, at once */
squares[0] = malloc(rows * cols * sizeof *squares[0]);
if (squares[0] != NULL) {
/* make nonzero row pointers point to the appropriate part */
for (i = 1; i < rows; i++)
squares = squares[0] + (i * cols);
} else {
/* give up row pointers since we cannot get data space */
free(squares);
squares = NULL;
}
}
*resultp = squares;
return squares != NULL;
}

This depends on the rest of the code: it makes the assumption that
there is no need to replace a given row without disturbing other
rows. (That is, if each row is separately malloc()ed, any row can
be replaced at any point:

old = squares[some_row];
new = malloc(...); /* some new row */
if (new == NULL) ... do something ...

/* insert desired processing here, then: */
squares[some_row] = new;
free(old);

If squares[some_row] is critically dependent on squares[0], as is
the case with the two-malloc() method, this is no longer possible.)

Yet another improvement is to get rid of the "bool" return value
entirely, and simply return the "world_square **" value (non-NULL
means success, NULL means failure). This eliminates the need for
the "world_square ***resultp" parameter.