4

I want to use a struct to contain some data and passing them between different functions in my program,this struct has to contain a dynamic 2D array (i need a matrix) the dimensions change depending on program arguments. So this is my struct :

    struct mystruct {
        int **my2darray;

    }

I have a function that read numbers from a file and has to assign each of them to a cell of the struct array.

I tried doing this :

    FILE *fp = fopen(filename, "r");
    int rows;
    int columns;
    struct mystruct *result = malloc(sizeof(struct mystruct));
    result->my2darray = malloc(sizeof(int)*rows); 
    int tmp[rows][columns];
    for(int i = 0;i<rows;i++) {
        for(int j = 0;j<columns;j++) {
            fscanf(fp, "%d", &tmp[i][j]); 
        }
        result->my2darray[i]=malloc(sizeof(int)*columns);
        memcpy(result->my2darray[i],tmp[i],sizeof(tmp[i]));
    }

But this is giving me a strange result : all the rows are correctly stored except for the first. (I'm sure that the problem is not in the scanning of file). While if i change the fourth line of code in this :

    result->my2darray = malloc(sizeof(int)*(rows+1)); 

it works fine. Now my question is why this happens?

Adl
  • 127
  • 3
  • 13
  • 2
    The code you show never assigns values to `rows` or `columns`. If this is your actual code, it is broken. If this is not actual code that reproduce the problem, you must provide a [mcve]. – Eric Postpischil Feb 15 '19 at 13:15
  • 1
    Not directly related, but I think `struct mystruct` should contain the number of rows and the number of columns. – Jabberwocky Feb 15 '19 at 13:17
  • 2
    Consider a `struct` something like `struct mystruct { size_t x,y; int my2darray[]; }` }` – chux - Reinstate Monica Feb 15 '19 at 13:18
  • @chux, that reads as "my2darray is an array of ints" but should be "array of rows of int". – Paul Ogilvie Feb 15 '19 at 13:27
  • The variables rows and columns are also dynamic And depend on program arguments , i've omitted The part of code doing this because It is neglectible – Adl Feb 15 '19 at 16:51
  • @PaulOgilvie The comment's "something like" were [weasel words](https://en.wikipedia.org/wiki/Weasel_word) to the general idea of a VLA. – chux - Reinstate Monica Feb 16 '19 at 00:18

3 Answers3

3

Here's an answer using some "new" features of the language: flexible array members and pointers to VLA.

First of all, please check Correctly allocating multi-dimensional arrays. You'll want a 2D array, not some look-up table.

To allocate such a true 2D array, you can utilize flexible array members:

typedef struct
{
  size_t x;
  size_t y;
  int flex[];
} array2d_t;

It will be allocated as a true array, although "mangled" into a single dimension:

size_t x = 2;
size_t y = 3;
array2d_t* arr2d = malloc( sizeof *arr2d + sizeof(int[x][y]) );

Because the problem with flexible array members is that they can neither be VLA nor 2-dimensional. And although casting it to another integer array type is safe (in regards of aliasing and alignment), the syntax is quite evil:

int(*ptr)[y] = (int(*)[y]) arr2d->flex;  // bleh!

It would be possible hide all this evil syntax behind a macro:

#define get_array(arr2d) \
  _Generic( (arr2d),     \
            array2d_t*: (int(*)[(arr2d)->y])(arr2d)->flex )

Read as: if arr2d is a of type array2d_t* then access that pointer to get the flex member, then cast it to an array pointer of appropriate type.

Full example:

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

typedef struct
{
  size_t x;
  size_t y;
  int flex[];
} array2d_t;

#define get_array(arr2d) \
  _Generic( (arr2d),     \
            array2d_t*: (int(*)[(arr2d)->y])(arr2d)->flex )

int main (void)
{
  size_t x = 2;
  size_t y = 3;

  array2d_t* arr = malloc( sizeof *arr + sizeof(int[x][y]) );
  arr->x = x;
  arr->y = y;
  

  for(size_t i=0; i<arr->x; i++)
  {
    for(size_t j=0; j<arr->y; j++)
    {
      get_array(arr)[i][j] = i+j;
      printf("%d ", get_array(arr)[i][j]);
    }
    printf("\n");
  }

  free(arr);
  return 0; 
}

Advantages over pointer-to-pointer:

  • An actual 2D array that can be allocated/freed with a single function call, and can be passed to functions like memcpy.

    For example if you have two array2d_t* pointing at allocated memory, you can copy all the contents with a single memcpy call, without needing to access individual members.

  • No extra clutter in the struct, just the array.

  • No cache misses upon array access due to the memory being segmented all over the heap.

Community
  • 1
  • 1
Lundin
  • 195,001
  • 40
  • 254
  • 396
  • Thanks, although other answers code works too, this is the most complete and interesting explanation. – Adl Feb 15 '19 at 17:57
  • "flexible array members is that they can neither ... nor 2-dimensional. " is unclear. Is not `typedef struct { size_t x; size_t y; int flex[][6]; } array2d6_t;` a counter example " flexible array members is 2-dimensional"? Perhaps you meant not flexible in 2 dimensions? – chux - Reinstate Monica Feb 16 '19 at 00:11
  • I'm getting an error while using the macro, `expression must have struct or union type but it has type "Matrix *"`. In my case, I have named `array2d_t*` to `Matrix*`. – Harshit Singh May 03 '22 at 22:43
  • @HarshitSingh So ask a separate question about that. You've made some syntax error, nobody can help you without seeing the code. – Lundin May 04 '22 at 06:19
1

The code above never sets rows and columns, so the code has undefined behavior from reading those values.

Assuming you set those values properly, this isn't allocating the proper amount of memory:

result->my2darray = malloc(sizeof(int)*rows);

You're actually allocating space for an array of int instead of an array of int *. If the latter is larger (and it most likely is) then you haven't allocated enough space for the array and you again invoke undefined behavior by writing past the end of allocated memory.

You can allocate the proper amount of space like this:

result->my2darray = malloc(sizeof(int *)*rows);

Or even better, as this doesn't depend on the actual type:

result->my2darray = malloc(sizeof(*result->my2darray)*rows);

Also, there's no need to create a temporary array to read values into. Just read them directly into my2darray:

for(int i = 0;i<rows;i++) {
    result->my2darray[i]=malloc(sizeof(int)*columns);
    for(int j = 0;j<columns;j++) {
        fscanf(fp, "%d", &result->my2darray[i][j]); 
    }
}
dbush
  • 205,898
  • 23
  • 218
  • 273
0

In your provided code example, the variables rows and columns have not been initialized before use, so they can contain anything, but are likely to be equal to 0. Either way, as written, the results will always be unpredictable.

When a 2D array is needed in C, it is useful to encapsulate the memory allocation, and freeing of memory into functions to simplify the task, and improve readability. For example, in your code the following line will create an array of 5 pointers, each pointing to 20 int storage locations: (creating 100 index addressable int locations.)

int main(void)
{
    struct mystruct result = {0}; 

    result.my2darray = Create2D(5, 20);

    if(result.my2darray)
    {
        // use result.my2darray 
        result.my2darray[0][3] = 20;// for simple example, but more likely in a read loop                         
        // then free result.my2darray
        free2D(result.my2darray, 5);
    }
    return 0;
}

Using the following two functions:

int ** Create2D(int c, int r)
{   
    int **arr;
    int    y;

    arr   = calloc(c, sizeof(int *)); //create c pointers (columns)
    for(y=0;y<c;y++)
    {
        arr[y] = calloc(r, sizeof(int)); //create r int locations for each pointer (rows)
    }
    return arr;
}

void free2D(int **arr, int c)
{
    int i;
    if(!arr) return;
    for(i=0;i<c;i++)
    {
        if(arr[i]) 
        {
            free(arr[i]);
            arr[i] = NULL;
        }
    }
    free(arr);
    arr = NULL;
}

Keep in mind that what you have created using this technique is actually 5 different pointer locations each pointing to a set of 20 int locations. This is what facilitates the use of array like indexing, i.e. we can say result.my2darray[1][3] represents the second column, forth row element of a 5X20 array, when it is not really an array at all.

int some_array[5][20] = {0};//init all elements to zero

Is what is commonly referred to in C an int array, also allowing access to each element via indexing. In actuality (Even though commonly referred to as an array.) it is not an array. The location of elements in this variable are stored in one contiguous location in memory.

|0|0|0|0|0|0|0|0|0|0|0|0|0|0|0|0|0|0...  (~ 82 more)

But C maintains the locations such that they are all indexable as an 2D array.

ryyker
  • 22,849
  • 3
  • 43
  • 87
  • See [Correctly allocating multi-dimensional arrays](https://stackoverflow.com/questions/42094465/correctly-allocating-multi-dimensional-arrays) – Lundin Feb 15 '19 at 14:08
  • @Lunden - I see more similarities than differences, except for one glaring thing. You are checking for success for each call to calloc, where I am not. That would be the one thing I would do here to improve. Is there something else you are pointing out that I have missed? – ryyker Feb 15 '19 at 14:12
  • 1
    Read the answer, not the question. I am pointing out that in your answer you are in fact not allocating any 2D arrays, making this code needlessly slow. – Lundin Feb 15 '19 at 14:16
  • 1
    @Lunden - Very nice. I have marked it a favorite, and will likely change the way I create memory. Thanks for sharing. Nice write up! (I am going to leave this answer as is, not because it is the best way, but because it does at least address a few other relevant points, i.e. the uninitialized variables, and more importantly to me, code readability. ) – ryyker Feb 15 '19 at 14:21