1
\$\begingroup\$

I want to use dynamically allocated data, corresponding to a 3D array in a C program. After using the data, I would like to properly deallocate the data again.

My approach is as follows:

#include <stdlib.h>
#include <stdio.h>

double ***arr3D_d( size_t dim1, size_t dim2, size_t dim3 ) {
    size_t  ii, jj;
    double  ***arr;

    arr     = calloc( (size_t)dim1, sizeof(double**) );

    for ( ii=0 ; ii < dim1 ; ++ii ) {
        arr[ii]  = calloc( (size_t)(dim1*dim2*dim3), sizeof(double*) );
        for ( jj=0 ; jj < dim2 ; ++jj) {
            arr[ii][jj] = calloc( (size_t)(dim2*dim3), sizeof(double) );
        }
    }
    return arr;
}


void free_arr3D_d( double ***arr, size_t dim1, size_t dim2 ) {
    size_t  ii, jj;

    for( ii=0 ; ii<dim1 ; ++ii ) {
        for ( jj=0 ; jj<dim2 ; ++jj ) {
            free( arr[ii][jj] );
        }
        free( arr[ii] );
    }
    free( arr );
}


int main( int argc, char *argv[] ) {

    size_t  ii, jj, kk, 
            dim1, dim2, dim3;

    double  ***arr3D;

    dim1    = 2;
    dim2    = 3;
    dim3    = 4;

    arr3D   = arr3D_d( dim1, dim2, dim3);

    for ( ii=0 ; ii<dim1 ; ++ii)
        for ( jj=0 ; jj<dim2 ; ++jj)
            for ( kk=0 ; kk<dim3 ; ++kk)
                printf( "arr3D[%ld][%ld][%ld] = %f (address: %p)\n", 
                        ii, jj, kk, arr3D[ii][jj][kk], 
                        (void*)&arr3D[ii][jj][kk] );

    free_arr3D_d( arr3D );
    return 0;
}

Compiling with gcc -Wall gives no errors and the code runs fine. Compiling with gcc -g and then testing with valgrind tells me that all memory is free'd.

So, everything seems to be fine. But, to be honest, I am not sure if I did the memory allocation part in a "good" way, as I don't understand it fully...

Any comments on my implementation are greatly appreciated.

Update1: I've just realized that there was an error in ***arr3D_d. Fixing the bug, and adding some error check, it should read as follows

double ***arr3D_d( size_t dim1, size_t dim2, size_t dim3 ){
    size_t   ii, jj;
    double   ***arr;

    arr = calloc( (size_t)dim1, sizeof(double**) );
    if (arr == NULL) {
        fprintf( stderr, "cannot allocate memory, probably because out of memory\n" );
        exit(0);
    }
    for ( ii=0 ; ii<dim1 ; ++ii ) {
        arr[ii] = calloc( (size_t)(dim2*dim3), sizeof(double*) );
        if (arr[ii] == NULL) {
            fprintf( stderr, "cannot allocate memory, probably because out of memory\n" );
            exit(0);
        }
        for ( jj=0 ; jj<dim2 ; ++jj ) {
            arr[ii][jj] = calloc( (size_t)(dim3), sizeof(double) );
            if (arr[ii][jj] == NULL) {
                fprintf( stderr, "cannot allocate memory, probably because out of memory\n" );
                exit(0);
            }
        }
    }
    return arr;
}
\$\endgroup\$
1
  • \$\begingroup\$ See Correctly allocating multi-dimensional arrays. As shown in the answer there, this function can be written in 1-2 lines if changed to use 3D arrays instead of slow pointer tables. \$\endgroup\$
    – Lundin
    Commented Dec 20, 2021 at 10:23

2 Answers 2

2
\$\begingroup\$

The array of pointers to arrays of pointers to arrays isn't the best representation of a 3D array. You get better locality and faster referencing by allocating dim1 * dim2 * dim3 elements, and then using arithmetic to access the required element.

I'd start with

struct array_3d {
    size_t length;
    size_t width;
    size_t depth;
    double *data;
}

and work from there.

Alternatively, allocate the data at the same time, using a flexible array member at the end of the struct:

struct array_3d {
    size_t length;
    size_t width;
    size_t depth;
    double data[];
}

That can allow you to make a single allocation.


arr     = calloc( (size_t)dim1, sizeof(double**) );

Why the no-op cast of dim1? Why are we zeroing storage we're immediately assigning to?

I'd expect

arr = malloc(sizeof *arr * dim1);

You've made absolutely no attempt to clean up and return an error indication when allocation fails. You'll discover that doing that properly is much easier when we have a linear array, rather than arrays of pointers.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for pointing me to an error check, which I had indeed completely forgotten. Hmm, interesting point about the linear array, instead of using pointers to arrays of pointers to arrays, I had no idea that this gives me faster referencing (certainly easier to free and check for an error). \$\endgroup\$
    – Alf
    Commented Dec 10, 2021 at 5:59
  • 1
    \$\begingroup\$ Thanks @chux - typo fixed \$\endgroup\$ Commented Dec 10, 2021 at 7:05
  • \$\begingroup\$ Two reason it's faster is that the single array is contiguous, so friendlier to caching and paging. Also, chasing pointers is prone to memory stalls - the processor can't do anything while it's waiting for the pointer value to arrive from memory. Multiplication by length and width isn't so prone to data stall. \$\endgroup\$ Commented Dec 10, 2021 at 7:16
2
\$\begingroup\$

Concerning updated code:

Unneeded cast

No need to cast dim1 to size_t. It is already type size_t.

// arr = calloc( (size_t)dim1, sizeof(double**) );
arr = calloc(dim1, sizeof(double**) );

Size to the object, not type

Rather than code the type, use the refenced object. Easier to code right, review and maintain.

// arr = calloc(dim1, sizeof(double**) );
arr = calloc(dim1, sizeof *arr);
// or 
arr = calloc(dim1, sizeof arr[0]);

Pedantic: Size 0?

If dim1 == 0, a return value of NULL is not certainly a problem. You may want to allow for that.

arr = calloc(dim1, sizeof arr[0]);
// if (arr == NULL) {
if (arr == NULL && dim1 > 0) {

Bug: Right size?

It really looks like a size of dim2 is needed here.

    // arr[ii] = calloc( (size_t)(dim2*dim3), sizeof(double*) );
    arr[ii] = calloc( dim2, sizeof(double*) );

Declare objects when needed

//size_t   ii, jj;
//...
//for ( ii=0 ; ii<dim1 ; ++ii ) {
for (size_t ii=0 ; ii<dim1 ; ++ii ) {

Repeated code

Rather than repeat code, consider a helper function.

static void* calloc_safe(size_t nmemb, size_t size) {
  // Uniformly handle zero cases here as desired. 
  if (nmemb > 0 && size > 0) {
    void *ptr = calloc(nmemb, size);
    if (ptr == NULL) {
      fprintf( stderr, "Cannot allocate memory %zu * %zu\n", nmemb, size);
      exit(0);
    }
    return ptr;
  }
  return NULL;
}

// Usage example - nice and clean.
double*** arr3D_d(size_t dim1, size_t dim2, size_t dim3) {
  double ***arr = calloc_safe(dim1, sizeof arr[0]);
  for (size_t ii = 0; ii < dim1; ++ii) {
    arr[ii] = calloc_safe(dim2, sizeof arr[ii][0]);
    for (size_t jj = 0; jj < dim2; ++jj) {
      arr[ii][jj] = calloc_safe(dim3, arr[ii][jj][0]);
    }
  }
  return arr;
}

calloc() vs malloc()

As code is going to fully populate that array, could use malloc().

I like the 2 term calloc()-like function signature as it allows the function to detect overflow in size versus having the caller do so.

static void* malloc_safe(size_t nmemb, size_t size) {
  // Demonstrative alternative zero case handling used here
  if (nmemb == 0 || size == 0) {
    nmemb = size = 1;
  }

  if (nmemb > SIZE_MAX/size) {
    fprintf( stderr, "Size too big %zu * %zu\n", nmemb, size);
    exit(0);
  }
  void *ptr = malloc(nmemb * size);
  if (ptr == NULL) {
    fprintf( stderr, "Cannot allocate memory %zu * %zu\n", nmemb, size);
    exit(0);
  }
  return ptr;
}

to properly deallocate the data again.

Freeing

As free(NULL) is OK, consider allowing free_arr3D_d(NULL, ...) and other pointers that may be NULL. This simplifies usage, especially in error handling.

void free_arr3D_d(double ***arr, size_t dim1, size_t dim2) {
  if (arr) {
    for (size_t ii = 0; ii < dim1; ++ii) {
      if (arr[ii]) {
        for (size_t jj = 0; jj < dim2; ++jj) {
          free(arr[ii][jj]);
        }
        free(arr[ii]);
      }
    }
    free(arr);
  }
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.