0

I have created this function to allocate a matrix dynamically. I would like to know if the function treats all the cases if something goes wrong (I mean free what malloc() has done).

void allocate(double*** m, int r, int c){
  int l;
  *m = (double**)malloc(sizeof(double*)*r);
  if(*m == NULL) return;
  for(l=0;l<r;l++){
    (*m)[l] = (double*)malloc(sizeof(double)*c);
    if((*m)[l] == NULL) {
        int l2;
        for(l2=0;l2<l;l2++)
            free((*m)[l2]);
        free(*m);
        *m = NULL;
        return; 
    }
  }
}
Anas M K
  • 107
  • 1
  • 1
  • 7
  • 1
    Instead of the 3ple pointer, just return from the function. Note that a matrix is an abstract construction. Your code allocates an array of pointer if anything. Oh, and make good use of whitespace, sothatyourcodeisnotunreadable. – Iharob Al Asimi Feb 16 '18 at 02:37
  • @IharobAlAsimi Thanks for your advice, I think I should use the 3ple pointer in this case. I cannot return a local pointer. – Anas M K Feb 16 '18 at 02:40
  • Why can't you return a local pointer? – Cris Luengo Feb 16 '18 at 03:03
  • 2
    You can't afford to return a pointer to local (non-static) data, but returning a pointer to dynamically allocated memory is safe. Thus `int *function1(void) { int array[] = { 9, 2, 4 }; return array; }` is bad — returning a pointer to local data that goes out of scope when the function returns. `int *function2(void) { int *array = malloc(3 * sizeof(int)); if (array == NULL) return array; array[0] = 0; array[1] = 2; array[2] = 4; return array; }` is returning a pointer to dynamically allocated memory and is safe. So is: `int *function3(void) { static int array[] = { 9, 2, 4 }; return array; }` – Jonathan Leffler Feb 16 '18 at 03:04
  • I would separate the deallocation into a separate function. You can then reuse it here and when you're finished using the matrix and need to deallocate it. – Cris Luengo Feb 16 '18 at 03:06
  • 1
    As it stands, your code correctly releases all the data that was previously allocated before returning if an allocation error occurs. You will need a function to release the allocated memory; it will need to know how many rows to release. You can use that in your error recovery, using `l` as the number of rows to be released. Note that [Three-star Programmer](http://c2.com/cgi/wiki?ThreeStarProgrammer) is not a compliment — if you have a 3D array, it's hard to avoid three stars, but you could return a `double **` safely, simplifying the notation in the function. – Jonathan Leffler Feb 16 '18 at 03:11
  • Also consider the merits of a single allocation of `r * c * sizeof(double)` bytes, and then setting the pointers to point to the right place in that array. It simplifies the memory release process enormously. – Jonathan Leffler Feb 16 '18 at 03:14
  • "I would like to know if the function treats all the cases if something goes wrong" --> It does not correctly handle the corner cases of `r==0` or `c==0` well. – chux - Reinstate Monica Feb 16 '18 at 03:18
  • Use common sense -> "*I cannot return a local pointer*", then how does `malloc()` work? Note that [this comment covers it very well](https://stackoverflow.com/questions/48819323/function-to-dynamically-allocate-matrix-and-treats-all-errors?noredirect=1#comment84642812_48819323) – Iharob Al Asimi Feb 16 '18 at 04:58

1 Answers1

1

As noted in a comment, within broad limits, if your code encounters an allocation error, you correctly release all the data that was allocated prior to the error.

You will need a function to release the allocated memory; it will need to know how many rows to release. You could use that in your error recovery, using l as the number of rows to be released.

void deallocate(double **matrix, int rows)
{
    for (int i = 0; i < rows; i++)
        free(matrix[i]);
    free(matrix);
}

That does not set the pointer passed in to null; you can use a three-star interface if you insist, but it probably isn't worthwhile (IMO — YMMV). This can then be used in the allocation code.

void allocate(double ***m, int r, int c)
{
    *m = (double **)malloc(sizeof(double *) * r);
    if (*m == NULL)
        return;
    for (int l = 0; l < r; l++)
    {
        (*m)[l] = (double *)malloc(sizeof(double) * c);
        if ((*m)[l] == NULL)
        {
            deallocate(*m, l);
            *m = NULL;
            return; 
        }
    }
}

There are those who are fanatical about 'no cast on malloc()' — I am not one of those, but I do make sure my code doesn’t compile in my development environment if <stdlib.h> was not included (so there was no declaration of malloc() et al viable when it is called). That makes it safe for user’s too, even if they don’t compile with the rigorous warning/error options I use.

Note too that Three-star Programmer is not a compliment. As suggested in the comments, you could use a different interface:

double **allocate(int r, int c)
{
    double **matrix = (double **)malloc(sizeof(double *) * r);
    if (m == NULL)
        return NULL;
    for (int l = 0; l < r; l++)
    {
        matrix[l] = (double *)malloc(sizeof(double) * c);
        if (matrix[l] == NULL)
        {
            deallocate(matrix, l);
            return NULL; 
        }
    }
    return matrix;
}

Note that because the allocation (and deallocation) functions use int and not size_t for the size parameters, you could get negative values. You could also get zeros provided — but if the size is zero, the code will 'work' safely, but the result will be unusable (no space allocated).

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278