>On Sat, 10 Feb 2007 04:09:02 +0100, John den Haan <no****@nospam.com>
>wrote:
>>bool allocate_viewport (world_square ***vis_squares, int rows, int cols)
{
int i;
*vis_squares = (world_square**) malloc(rows * sizeof(world_square*));
In article <7l********************************@4ax.com>,
Barry Schwarz <sc******@doezl.netwrote:
>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[i] = malloc(cols * sizeof *squares[i]);
if (squares[i] == NULL) {
failed = true;
break;
}
}
if (failed) {
/* release the ones we successfully allocated */
while (--i >= 0)
free(squares[i]);
/* 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[i] = 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.
--
In-Real-Life: Chris Torek, Wind River Systems
Salt Lake City, UT, USA (40°39.22'N, 111°50.29'W) +1 801 277 2603
email: forget about it
http://web.torek.net/torek/index.html
Reading email is like searching for food in the garbage, thanks to spammers.
