3

I'm making a program where I have to work constantly with matrices in functions, this is one of the many functions, this function is supposed to take open an external file which is a data set, where the data is separated with tabulations, it opens the file and saves the data in a matrix M, I know this matrix is composed of 6 columns, but the row number is unknown, I know the error is where I declare the matrix, it has to be declared with pointers since the function returns the matrix.

//type float** since it will return a matrix
float **carga_archivo(char *nombre_archivo)       
{
  float **M=(float **)malloc(6*sizeof(float*));  //error should be here
  int i=0;
  FILE *archivo;                      //FILE type pointer to open the external file
  archivo=fopen(nombre__archivo,"r"); //Opens the file in the address indicated
                                      //"nombre_de_archivo" is a variable
  while(!feof(archivo))               //Browses the file row per row till the end of it
  {
      //saves the data in its corresponding place in the matrix
      fscanf(archivo,"%f\t%f\t%f\t%f\t%f\t%f\n",
          &M[0][i],&M[1][i],&M[2][i],&M[3][i],&M[4][i],&M[5][i]);
      i++;
  }
  tam=i;
  fclose (archivo);                  //closes the file
  return M;
}

What I need is the right way to declare the matrix.

P.S. I documented the main lines in the code in case it could help someone who needs something similar.

Any correction is welcome.

Update: Applied some changes proposed in comments, and worked better, here is the new code I nade for that function

float **carga_archivo(char *nombre_archivo)
{
int i=0;
float P[300][6];
FILE *archivo;
archivo=fopen(nombre_archivo,"r");
 while(!feof(archivo))
{
i++;
       //this was just so the feof function could browse row per row 
       //instead of character per character
    scanf("%f\t%f\t%f\t%f\t%f\t%f\n",
               &P[0][i],&P[1][i],&P[2][i],&P[3][i],&P[4][i],&P[5][i]);
    printf("%i\n",i);
}
tam=i;
printf("%i",tam);
int filas = 6;
int columnas = tam;
float **M;

M = (float **)malloc(filas*sizeof(float*));

for (i=0;i<filas;i++)
    M[i] = (float*)malloc(columnas*sizeof(float));

for (i = 0; i < columnas; ++i)
    fscanf(archivo,"%f\t%f\t%f\t%f\t%f\t%f\n",
             &M[0][i],&M[1][i],&M[2][i],&M[3][i],&M[4][i],&M[5][i]);
fclose (archivo);
return M;
}

The new problem is when the function is called, the program actually compiles, but when its running and the function is called the program crashes and stops. Here Is the part of the code that calls to that function.

int main()
{
int i,j;
char *nombre_archivo="Agua_Vapor.txt";
float **agua_vapor=carga_archivo(nombre_archivo);
for (i = 0; i < 6; i++)
{
    for (j = 0; i < tam; i++)
        printf("%f   ", agua_vapor[i][j]);
    printf("\n");
}
return 0;
}
  • 2
    `char nombre_archivo` is wrong. A filename has more than one character. Also, if the number of rows is unknown -- you need to get that information back to the caller, perhaps as the return value (while passing a pointer to the function which will point to the array of floats after the function returns. – John Coleman Dec 06 '16 at 02:03
  • thank you, I just edited the first issue you mention, but I think I don't follow you in the second part, I'm not very familiar with pointers, maybe an example could help me a little – Felipe Tafoya Loeza Dec 06 '16 at 02:19
  • 2
    If you don't know the number of rows in advance, you'll need to scan through the file at least once to get that number, *then* allocate the *total* amount of memory need to store your data, and then read the data from the file into your matrix. That requires reading through the file twice. –  Dec 06 '16 at 02:22
  • To add to Evert's answer, You can read the file once but will have to store your data in local storage. You can define a separate function that can add values to a dynamically growing array. Once the copy from file is complete. Allocate a two dimension array [row][column] and copy the content. – shobhit Dec 06 '16 at 02:24
  • NB: for a matrix, you may want to prefer to allocate N x M cells of memory as a *linear* array, instead of N columns each pointing to a single row (resulting in a pointer to a pointer). –  Dec 06 '16 at 02:24
  • @shobhit but how would you store that data in local storage if you don't know the file size in advance? –  Dec 06 '16 at 02:25
  • @evert, updated my comment. You can know the size by keeping a counter when you are reading the file. You can store the content in a dynamically growing array by making a function akin to C++ vector. This is kind of trade off between reading file twice or making a dynamically growing vector. – shobhit Dec 06 '16 at 02:33
  • Note that [`while (feof(file))` is always wrong](http://stackoverflow.com/questions/5431941/while-feof-file-is-always-wrong). – Jonathan Leffler Dec 06 '16 at 05:16

1 Answers1

2

Your program has undefined behaviour, because you're populating memory referenced by uninitialised pointers.

Since you know there are always 6 columns, a simple approach is to store the matrix as row-major instead of column-major (your example is column-major). This means you can store the matrix data as one large piece of memory and use realloc when necessary. You may want to make a simple struct for this too.

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

Then create it dynamically.

struct matrix * matrix_alloc( int rows, int cols )
{
    int i;

    struct matrix * m = malloc(sizeof(struct matrix));
    m->rows = rows;
    m->cols = cols;
    m->data = malloc(rows * sizeof(float*));
    m->data[0] = malloc(rows * cols * sizeof(float));

    for( i = 1; i < rows; i++ ) {
        m->data[i] = m->data[i-1] + cols; 
    }
    return m;   
}

void matrix_free( struct matrix * m )
{
    free( m->data[0] );
    free( m->data );
    free( m );
}

Now, when you decide you need to add storage for more rows:

void matrix_set_row_dimension( struct matrix * m, int rows )
{
    float **new_index, *new_block;
    new_index = realloc(m->data, rows * sizeof(float**));
    new_block = realloc(m->data[0], rows * m->cols * sizeof(float));
    if( new_index && new_block )
    {
        int i = m->rows;
        m->rows = rows;
        m->data = new_index;

        /* if block address changed, prepare to reindex entire block */
        if( m->data[0] != new_block )
        {
            m->data[0] = new_block;
            i = 1;
        }

        /* reindex */
        for( ; i < rows; i++ ) {
            m->data[i] = m->data[i-1] + cols;
        }
    }
}

So, now as you populate the matrix...

struct matrix * m = matrix_alloc( 10, 6 );  /* Start with 10 rows */
int row = 0;
while( 1 ) {
    /* Double matrix row count if not large enough */
    if( row == m->rows )
    {
        matrix_set_row_dimension( m, m->rows * 2 );

        /* Check for error here */
    }

    /* Now the matrix has enough storage to continue adding */
    m->data[row][0] = 42;
    m->data[row][1] = 42;
    m->data[row][2] = 42;
    m->data[row][3] = 42;
    m->data[row][4] = 42;
    m->data[row][5] = 42;

    row++;     
}
chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
paddy
  • 60,864
  • 6
  • 61
  • 103
  • 1
    Note: this is not hardened code. There would need to be proper handling of all reallocation failure scenarios, as well as handling initial allocation failures. The code here is purely illustrative of a reasonable approach. – paddy Dec 06 '16 at 02:45
  • 1
    Note: `m->data[0] = malloc(sizeof(float) * rows * cols);` has a small advantage over `m->data[0] = malloc(rows * cols * sizeof(float));` in that `sizeof(float) * rows * cols` will typically not overflow as soon as `rows * cols * sizeof(float)`. – chux - Reinstate Monica Dec 06 '16 at 03:19