2

I'm still new to C, malloc, and all that jazz, so I decided to write this to learn some more skills. The idea is, I'm reading in a bunch of ints from a file and putting them into a matrix (2d array). The start of the file says how many rows and columns there are, so it reads those numbers in and uses malloc to set up the 2d array.

int read_matrix(FILE *mat, int ***Z, int *x, int *y) 
{
    int i = 0;
    int x_temp = 0;
    int y_temp = 0;

    if (fscanf(mat, "%d %d", &(*x), &(*y)) == EOF){
        printf("File is not big enough to contain a matrix\n");
        return -1;
    }

    printf("About to malloc %d\n", *x);

    *Z = (int**) malloc(*x * sizeof(int*));
    while (i < *x) {
        printf("mallocing %d\n", i);
        *Z[i] = (int*) malloc(*y * sizeof(int));
        printf("malloced\n");
        ++i;
    }

    printf("Malloc complete\n");

    /*Other unimportant code*/
}

The output reads:

About to malloc 3 
mallocing 0 
malloced 
mallocing 1
Segmentation fault

So it's not mallocing anything but one int** in Z.. I think?

I'm very new to C, so I'm not sure if I've made some little mistake, or if I'm really going about this whole thing incorrectly. Any thoughts? Thanks!

Mark Elliot
  • 75,278
  • 22
  • 140
  • 160
cost
  • 4,420
  • 8
  • 48
  • 80

3 Answers3

3

The [] operator binds more closely than the unary * operator. Try changing *Z[i] to (*Z)[i] and see if your code behaves.

As a side note, it's also quite common in C to malloc a single array of (sizex*sizey) size, for a matrix and then index it as arr[x*sizey + y] or arr[y*sizex + x]. That more closely mimics what the language does with static arrays (e.g. if you declare int foo[10][10], all 100 ints are contiguous in memory and nowhere is a list of 10 int*'s stored.

caf
  • 233,326
  • 40
  • 323
  • 462
Walter Mundt
  • 24,753
  • 5
  • 53
  • 61
  • Woot, that did it, malloc is complete (new with a new seg fault afterwards). Thanks! I'll mark you as the answer once the time limit is up – cost Feb 03 '11 at 01:33
  • Note that you can `malloc` a single two dimensional array and still keep the same syntax as `int foo[10][10]`, as long as all but the first dimension are integer constants: `int (*foo)[10] = malloc(10 * sizeof foo[0]);` – caf Feb 03 '11 at 01:36
  • @cost (& @caf): If you have a C99 complying compiler none of the dimensions must be a compile time constant, this is called variable length array, VLA, respectively the pointer is then a so called variably modified type (VM). This is sometimes frowned upon when allocated on the stack, but if you allocate it as you do with `malloc` on the heap, this is the simplest solution to handle dynamic sizes. – Jens Gustedt Feb 03 '11 at 07:42
2

I agree with both Walter and AndreyT. This is just some additional information.

Note that you can get away with just two malloc() calls, rather than *x + 1 - one big block for the ints themselves, and one for the row index.

*Z = malloc(*x * sizeof (*Z)[0]);
(*Z)[0] = malloc(*x * *y * sizeof (*Z)[0][0]);
for (i = 1; i < *x; i++) {
    (*Z)[i] = (*Z)[0] + i * *y;
}
caf
  • 233,326
  • 40
  • 323
  • 462
1

As Walter correctly noted in his answer, it should be (*Z)[i] = ..., not *Z[i] = ....

On top of that I'd suggest getting rid of that dereference/typecast hell present in your source code. Don't cast the result of malloc. Don't use type names under sizeof. Expressing it as follows

 *Z = malloc(*x * sizeof **Z);
 ...
 (*Z)[i] = malloc(*y * sizeof *(*Z)[i]);

wil make your code type-independent and much more readable.

A separate question is what on Earth made you use &(*x) in fscanf. Is this some kind of bizarre coding standard?

caf
  • 233,326
  • 40
  • 323
  • 462
AnT stands with Russia
  • 312,472
  • 42
  • 525
  • 765
  • It will also make your code [not compile as C++](http://stackoverflow.com/questions/3477741/why-does-c-require-a-cast-for-malloc-but-c-doesnt) so I wouldn't recommend this. – Walter Mundt Feb 03 '11 at 01:38
  • You mean it won't compile on C++ if I cast the result of malloc? – cost Feb 03 '11 at 01:56
  • &(*x) is the kind of coding you're doing when you're not thinking very well when you're coding, heh... – cost Feb 03 '11 at 02:02
  • Other way around. To compile on C++, you *must* cast the result of malloc, because `void*` can only convert implicitly to other pointers in plain C. – Walter Mundt Feb 03 '11 at 02:12
  • @Walter Mundt: Firstly, this is a C question. C++ has noting to do with it. Secondly, the issue of being C-C++ cross compilable is mostly relevant to header file contents, meaning that it is rarely matters for function bodies (aside from inline functions). Basically, the issue of not casing `malloc` never comes up as a C-C++ cross-compilability issue, aside from macros. – AnT stands with Russia Feb 03 '11 at 05:46
  • Fair enough. I still think it's important for as many people as possible to know the differences in general, but your point is well taken. For context, I've just run into issues with C++ code-bases where people had left off various casts like this and ignored warnings which then became errors in future compiler versions. While not relevant to this question, that's why I tend to want anyone working in C to know what they'd have to do differently if they had to get their C code to compile as C++. – Walter Mundt Feb 03 '11 at 17:46
  • @William The *reason* a cast is needed in C++ is because malloc is not the preferred way to allocate memory in C++ and so Stroustrup chose strong typing for void*. – Jim Balter Feb 08 '11 at 11:50
  • @caf: Thanks for the edits. Apparently, my version was not so much more readable as to make me notice the errors :) – AnT stands with Russia Feb 08 '11 at 16:26