Posts Tagged ‘Programming’

Don’t write functions to return local arrays

December 23, 2009

Consider the following function (taken from Forgotten Times II):

char* CClassicRPGDoc::levelLocationID(int which_level)
{

char center_location_ID[LABELLENGTH];
int center_location_index = current_location;

char retval[LABELLENGTH];

strcpy(center_location_ID, location_IDs[current_location]);

int counter;
if(which_level > 0)
{
for(counter = 1; counter <= which_level; counter++)
{
strcpy(center_location_ID, locations[center_location_index].getTop().getLocationID());
center_location_index = getLocationIndex(center_location_ID);
}
}
if(which_level = which_level; counter–)
{
strcpy(center_location_ID, locations[center_location_index].getBase().getLocationID());
center_location_index = getLocationIndex(center_location_ID);
}
}
if(which_level != 0)
{
ASSERT(true);
}
strcpy(retval, center_location_ID);
return retval;
};

The above function is used in the display of multi-level buildings and was causing me no end of grief. For some reason, instead of returning what I expected (i.e., the ID of the specified floor of a building) it was consistently returning the ID of the floor (i.e., the one the character was in). Even worse, its behavior changed when I ran it through the debugger, and when I traced the values of the variables all the way to the end of the function, they looked the way they were supposed to.

The problem, of course, was the statement in bold text. While the retval[] string had the right value all the way to the end of the function, once the function exited, the memory that string had occupied was deallocated. As a result, rather than passing back the string I intended, I was passing back a memory location which, by that time, was being used for something else (apparently for storing the ID of the current floor, though I have no idea why). Very annoying.

While there are a number of ways to solve the problem (including reworking the logic so that the problem didn’t come up in the first place), I chose the one that was easiest for me, and made retval[] a static variable. Not the most artful way of doing it, but, since static variables are maintained between function calls, it did the job. The final results are that the last upload of Forgotten Times II has a reasonably functional implementation of multi-story building display, and that I’ve learned to avoid writing functions to return local arrays.

Advertisements