How can I free a growable (using realloc) dynamic array within a struct?-CodePudding

I'm revisiting C after quite a long break, and have been utilising growable arrays specifically to help me get back into manual memory management. I use modified code from, and credit to, University of Exeter).

I'm trying to free the individually allocated elements from an (in this case integer) array within a struct, but am having trouble in doing so.

First, I create a struct to hold an integer array, the current number of elements, and the buffer size to keep track of allocated memory.

typedef struct int_array
{
    int *values;
    int numValues;
    int bufferLen;
} IntArray;

The initial struct is then created using the following:

IntArray *new_int_array(void)
{
    IntArray *intArr;

    intArr = calloc(1, sizeof *intArr);
    intArr->bufferLen = intArr->numValues = 0;
    intArr->values = NULL;

    return intArr;
}

Finally, to add elements to the array:

void add_element_to_array(IntArray *intArr, int 
    values)
{
    // Check to see if array extension needed
    if (intArr->numValues == intArr->bufferLen) {
        intArr->bufferLen  = GROWBY; // #define GROWBY 16
        intArr->values = realloc
            (intArr->values, intArr->bufferLen * 
                sizeof intArr->values);
    }

    intArr->values[intArr->numValues] = 
        values;
    intArr->numValues  ;
}

I'm used to freeing a single-malloc'd/calloc'd array, but from what I have read looping through all elements and freeing each one (one free per one allocation) would be the correct way of going about it, but I can't seem to get it right. I'm calling a helper function to free the memory:

void free_struct(IntArray *intArr)
{
    for(int i = 0; i < intArr->bufferLen; i  ){
        free(intArr->values[i]);
    }
}

I could paste in the numerous errors I get while playing around with the above free_struct function, but I'd be here forever. They range from expecting void * to invalid type arguments, to double frees and others beside.

EDIT: I neglected to mention the biggest issue I faced, and that was a memory leak according to Valgrind. The accepted answer solved the problem, as correctly dereferencing intArr->values allowed me to free the same intArr->values and then intArr itself.

CodePudding user response：

There's a problem in this code when adding:

if (intArr->numValues == intArr->bufferLen) {
    intArr->bufferLen  = GROWBY; // #define GROWBY 16
    intArr->values = realloc
        (intArr->values, intArr->bufferLen *
            sizeof intArr->values);
}

This is on the way toward the fix:

if( arr->nVal == arr->bufLen) {
    arr->bufLen  = GROWBY; // #define GROWBY 16
    arr->vals = realloc( arr->vals, arr->bufLen * sizeof *arr->vals );
                                            // NOTICE    ^
}

The OP code lacked a dereferencing asterisk in the sizeof term of the expression.

Notice how much easier this is to read when an involved statement is not spread across multiple lines of source code. I recommend using meaningful-but-short variable names. Source code needs to be 'scannable', not 'readable beside the fire on a winter's evening.'

Notice that the size of an element is the size of an integer, not a pointer. This will become important when the stored integers change to stored structs.

There's usually no alternative, when that alloc family fail in their task but to exit gracefully. To continue processing with a NULL pointer (until a crash) is not graceful.

if( arr->nVal == arr->bufLen) {
    arr->bufLen  = GROWBY; // #define GROWBY 16
    arr->vals = realloc( arr->vals, arr->bufLen * sizeof *arr->vals );
    if( arr->vals == NULL ) {
        fprintf( stderr, "realloc() failure\n" );
        exit( 1 );
    }
}

Now that that is cleared up, the 'free()` problem should be easier to understand.

The one pointer to the array points at a single contiguous block of memory. You only need one free( arr->vals ); to release the entire block.

While this 'grows' the array in increments of GROWBY, you might want to experiment with removing a random element (involving shifting subsequent elements upward in the array; see memmove()) and even shrinking the array (realloc() again) when there are SHRINKBY unused elements at the bottom of the array...

EDIT:
Because calloc() guarantees the allocated bytes will be set to 0, this:

IntArray *new_int_array(void)
{
    IntArray *intArr;

    intArr = calloc(1, sizeof *intArr);
    intArr->bufferLen = intArr->numValues = 0;
    intArr->values = NULL;

    return intArr;
}

can be reduced to:

IntArray *new_int_array(void)
{
    IntArray *arr = calloc(1, sizeof *arr);
    if( arr == NULL ) {
        fprintf( stderr, "calloc() failed\n" );
        exit( 1 );
    }
    return arr;
}