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!

Improve this question
2
  • &(*x) is the same as just x. Commented Feb 3, 2011 at 1:37
  • Ahhhhh yea, just ignore the &(*x) as me being an idiot hahah Commented Feb 3, 2011 at 2:04

3 Answers 3

Reset to default
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.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

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
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]);
@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.
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;
}
Improve this answer

1 Comment

Good catch! Note that this changes the contract though, since the caller has to know to only call two free's.
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?

Improve this answer

8 Comments

It will also make your code not compile as C++ so I wouldn't recommend this.
You mean it won't compile on C++ if I cast the result of malloc?
&(*x) is the kind of coding you're doing when you're not thinking very well when you're coding, heh...
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: 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.
|

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.