1

I'm trying to load file from non-main function with pointer to pointer (2D matrix) as argument in ANSI C.

Approach with l-value function is correct:

float **loadArray(int *rows, int *columns) {
    float **array;
    FILE *instream; // input file pointer
    char infile[21] = {}; //20 chars max filename length
    int i = 0, j = 0; //iterators
    printf("filename to load data from: ");
    scanf(" %20s", infile);
    if (!(instream = fopen(infile, "r"))) {
        perror("fopen() error");
        exit(-1);
    }
    fread(rows, sizeof(int), 1, instream);
    fread(columns, sizeof(int), 1, instream);
    fprintf(stdout, "\narray(%d,%d):", *rows, *columns); //OK
    // allocation of vertical array containing rows pointers.
    if (!(array = (float**)malloc((*rows) * sizeof(float*)))) {
        printf("vertical malloc() error");
        exit(-1);
    }
    for (i = 0; i < (*rows); i++) 
        // for every row allocate columns space
        if (!(array[i] = (float*)malloc((*columns) * sizeof(float)))) {
            printf("horizontal malloc() error");
            exit(-1);
        }
    for (i = 0; i < (*rows); i++)
        for (j = 0; j < (*columns); j++)
            fread((&array[i][j]), sizeof(float), 1, instream);
    fclose(instream);
    return array;
}

int main() {
    int i = 0;
    int rows = 0, columns = 0;
    float **myarray;
    myarray = loadArray(&rows, &columns);
    ...
    for (i = 0; i < rows; i++)
        free(myarray[i]);
    free(myarray);
}

But I'm trying to be consistent when reading from file and rows, columns are passed as addresses to pass the array in the same way:

int loadArray2(float ***array, int *rows, int *columns) {
    FILE *instream; // input file pointer
    char infile[21] = {}; //20 chars max filename length
    int i = 0, j = 0; //iterators
    printf("filename to load data from: ");
    scanf(" %20s", infile);
    if (!(instream = fopen(infile, "r"))) {
        perror("fopen() error");
        exit(-1);
    }
    fread(rows, sizeof(int), 1, instream);
    fread(columns, sizeof(int), 1, instream);
    fprintf(stdout,"\narray(%d,%d):", *rows, *columns); //OK
    // allocation of vertical array containing rows pointers.
    if (!(*array = (float**)malloc((*rows) * sizeof(float*)))) {
        printf("vertical malloc() error");
        exit(-1);
    }
    for (i = 0; i < (*rows); i++) 
        // for every row allocate columns space
        if (!(*array[i] = (float*)malloc((*columns) * sizeof(float)))) {
            printf("horizontal malloc() error");
            exit(-1);
        }
    for (i = 0; i < (*rows); i++)
        for (j = 0; j < (*columns); j++)
            fread((array[i][j]), sizeof(float), 1, instream);
    fclose(instream);
    return 0;
}

int main() {
    int rows = 0, columns = 0;
    float **myarray;
    loadArray2(&myarray, &rows, &columns);
    ...
    for (i = 0; i < rows; i++)
        free(myarray[i]);
    free(myarray);
}

But this approach failed. I guess I made mistake in malloc() calling either no error or warnings, or my logic is bad, I admit I'm lost...

Thanks for some tips.

chqrlie
  • 131,814
  • 10
  • 121
  • 189
zelenka-jan
  • 13
  • 2
  • 1
    You read binary data from the file, but you open it in *text* mode. That means some bytes in the input file could be translated to other bytes when you read them. To read raw binary data you need to open in binary mode. – Some programmer dude May 05 '20 at 09:33
  • 2
    A few other points: Don't try to cram so much statements or expressions onto a single line, that makes the code much harder to read and understand. One statement per line, please. And also please add some empty line to divide the code into paragraphs. Also in C you [should not cast the result of `malloc`](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). And you generally don't want to be called a [three star programmer](https://wiki.c2.com/?ThreeStarProgrammer). – Some programmer dude May 05 '20 at 09:36
  • 2
    This is very obfuscated code. You need to adopt a more conventional coding style if you want other people to read your code, or if you wish to be able to debug it yourself. – Lundin May 05 '20 at 09:39
  • 2
    As for the bug, it's simple operator precedence: `*array[i]` -> `(*array)[i]`. – Lundin May 05 '20 at 09:41
  • By the way, why have the second function, `loadArray2`, return anything at all? If there's a failure it will `exit` (by the way don't use `-1` for `exit`, use a [standard exit code](https://en.cppreference.com/w/c/program/EXIT_status) like `EXIT_FAILURE`), and if the function succeeds you always return `0`. – Some programmer dude May 05 '20 at 09:47
  • Hello, thank you all for your answers and useful tips and corrections, yes, the code is little bit fuzzy ;-) The prefix was the problem. SOLVED – zelenka-jan May 05 '20 at 13:18

2 Answers2

1

In the failing function a major problem is that array is a pointer to a float **, a pointer you need to dereference to get the original float ** variable. Just like you do with the rows and columns arguments.

So instead of doing e.g. array[i][j] you need to do (*array)[i][j].

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
0

When you use a triple pointer in loadArray2, remember that postfix operators bind more tightly than prefix operators. So you should write (*array)[i] when storing the pointer to the allocated array of float and &(*array)[i][j] when reading the float value.

Note that there are other problems in your code:

  • char infile[21] = {}; is not a valid syntax in C (it is in C++ and might be supported by your compiler as an extension or maybe because you are compiling this code as C++). Write char infile[21] = ""; instead.
  • Since you are reading binary data from the file, you should open it in binary mode: fopen(infile, "rb"). It might not make a difference on linux and Mac/OS, but is definitely an error on legacy systems.
  • You should check for read errors.

  • You could read the matrix lines in one call (below the code for the first option):

    for (i = 0; i < (*rows); i++) {
    if (fread(array[i], sizeof(float), *cols, instream) != *cols) {
        // handle the read error
    }
}

Whether to use the first or second approach is debatable:

  • in the first case, it is easy to return an error with a NULL pointer, so passing the indirect 2D array pointer by reference seems unnecessary. It is a more idiomatic way to return this pointer in C. If more information is needed regarding the error, you could take a pointer to the error code as an extra argument.
  • in the second case, the error code can be returned directly for more precise diagnostics, but the triple pointer is error prone, as you just experienced. Such types are discouraged. Don't be a tree star programmer :)

An alternative approach is to encapsulate the pointer and the rows and columns attributes in a structure and pass a pointer to the destination structure.

Here is a modified version with this approach:

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

struct matrix {
    int cols, rows;
    float **data;
};

void matrix_reset(struct matrix *mat) {
    int i;
    if (mat->data) {
        for (i = 0; i < mat->rows; i++)
            free(mat->data[i]);
        free(mat->data);
        mat->data = NULL;
    }
    mat->rows = 0;
    mat->cols = 0;
}

int matrix_load(struct matrix *mat) {
    FILE *instream; // input file pointer
    char infile[257] = ""; //256 chars max filename length

    printf("filename to load data from: ");
    if (scanf(" %256s", infile) != 1) {
        perror("invalid input");
        return -1;
    }
    if (!(instream = fopen(infile, "rb"))) {
        perror("fopen() error");
        return -2;
    }
    if (fread(&mat->rows, sizeof(int), 1, instream) != 1
    ||  fread(&mat->cols, sizeof(int), 1, instream) != 1) {
        perror("fread() error");
        fclose(instream);
        return -3;
    }
    fprintf(stdout, "\narray(%d,%d):", mat->rows, mat->cols);
    // allocation of vertical array containing rows pointers.
    if (!(mat->data = calloc(mat->rows, sizeof(*mat->data)))) {
        printf("vertical malloc() error");
        fclose(instream);
        return -4;
    }
    for (int i = 0; i < mat->rows; i++) {
        // for every row allocate columns space
        if (!(mat->data[i] = calloc(mat->cols, sizeof(*mat->data[i])))) {
            printf("horizontal malloc() error");
            matrix_reset(mat);
            fclose(instream);
            return -5;
        }
    }
    for (i = 0; i < mat->rows; i++)
        if (fread(&mat->data[i], sizeof(*mat->data[i]), mat->cols, instream) != mat->cols) {
            printf("horizontal malloc() error");
            matrix_reset(mat);
            fclose(instream);
            return -6;
        }
    }
    fclose(instream);
    return 0;
}

int main() {
    struct matrix m = { 0, 0, NULL};

    if (!matrix_load(&m)) {
       ...
    }
    matrix_reset(&m);
    return 0;
}
chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • Hello, thank you so much for the struct example, I was not so brave to try it as first way :-) and also for explaining me my faults. – zelenka-jan May 05 '20 at 13:20