Arrays of structures, dynamic allocation

Question

I was wondering if my dynamic allocation has been done correctly as I’m currently getting these errors:

and

I wanted an array of structures and each structure is holding four elements.

struct train
{
    char* start;
    char* destination;
    int start_time;
    int arrival_time;
};

int i = 0, j, file, time_depature, time_arrival, rows = 0;
char temp_station[20], temp_destination[20];
FILE* times;
struct train *train_array;

        do
        {
            /*Take in four elements from the file and measure how many rows there are in the file from dynamic allocation*/
            file = fscanf(times, "%s %s %d %d", temp_station, temp_destination, &time_depature, &time_arrival);
            rows++;

        } while (file != EOF);

    /*Array of structs using dynamic allocation*/
    train_array = (struct train*)malloc(sizeof(struct train_array)*rows);

    do
    {
        /*Take in four elements from the file*/
        file = fscanf(times, "%s %s %d %d", temp_station, temp_destination, &time_depature, &time_arrival);
        /*makes sure does not go to the end of file*/
        if (file != EOF)
        {
            /*allocates the element in the array*/
            train_array[i] = (struct train*)malloc(sizeof(struct person_array));
            /*copies the string into the array*/
            strcpy(train_array[i].start, temp_station);
            i++;
        }
    } while (file != EOF);

    /*closes the file*/
    fclose(times);

Answer 1

You code has a number of issues, but let's back off of that for a minute and talk about memory management in general.

A common (and my preferred) idiom for allocating memory is something like

T *p = malloc( sizeof *p * N ); 

or

T *p = NULL;
...
p = malloc( sizeof *p * N );

which will allocate enough space to hold N elements of type T. Note that there is no cast on the result of malloc; as of the 1989 standard, the cast is no longer necessary¹, and under C89 compilers it will suppress a diagnostic if you don't have a declaration for malloc in scope (i.e., you forgot to include stdlib.h). C99 got rid of implicit int declarations, so that's not as big of an issue anymore, but a lot of compilers default to the 1989 standard.

Note that I also use sizeof *p to get the size of each element; the expression *p has type T, so it follows that sizeof *p is equivalent to sizeof (T)². Not only does this reduce visual clutter, it saves you a minor maintenance headache of the type of *p ever changes, say from int to long or float to double.

C isn't your mother and won't clean up after you; you are responsible for deallocating any memory you allocated when you are done with it.

Note that you can resize a dynamically-allocated buffer with the realloc library function. Here's a typical usage:

#define INITIAL_SIZE ... // some initial value
...
size_t arraysize = INITIAL_SIZE;
size_t counter = 0;
T *array = malloc( sizeof *array * arraysize );
...
while ( we_need_to_store_more_data )
{
  if ( counter == arraysize )
  {
    /**
     * We've run out of room to store new things, so
     * we need to extend the array; one common
     * strategy is to double the size of the array
     * each time.
     */
    T *tmp = realloc( sizeof *array * (2 * arraysize) );
    if ( tmp ) 
    {
      array = tmp;
      arraysize *= 2;
    }
  }
  array[counter++] = new_value;
}

Note that for a type like

struct foo
{
  char *name;
  char *address;
  int value;
};

you have to allocate space for name and number separately from the instance of struct foo itself:

struct foo *farr = malloc( sizeof *farr * N );
...
farr[i].name    = malloc( sizeof *farr[i].name * name_length );
farr[i].address = malloc( sizeof *farr[i].address * address_length);

You have to be really careful when you get into multiple allocation and deallocation steps like this; it's a really good idea to abstract these operations out into their own functions, like:

if ( !add_data( &foo[i], newName, newAddress, newValue ))
  // report error

which would look something like

int add_data( struct foo *elem, const char *name, const char *addr, int val )
{
  assert( elem != NULL );
  elem->name = malloc( strlen( name ) + 1 );
  elem->address = malloc( strlen( addr ) + 1 );

  if ( !elem->name || !elem->address ) // Make sure memory allocation
  {                                    // succeeded before trying to
    free( elem->name );                // use name or address; malloc returns
    free( elem->address );             // NULL on failure, and free(NULL)
    return 0;                          // is a no-op.  We want to undo any
  }                                    // partial allocation before returning
                                       // an error.
  strcpy( elem->name, name );
  strcpy( elem->address, addr );
  elem->value = val;

  return 1;
}

Now let's look at the specific issues in your code:

train_array = (struct train*)malloc(sizeof(struct train_array)*rows);
                                           ^^^^^^^^^^^^^^^^^^^

The name of the type is struct train; the name of the object you're trying to allocate is train_array. You've crossed the two, which is a problem. Going by my advice above, rewrite that to

train_array = malloc( sizeof *train_array * rows );

This line

train_array[i] = (struct train*)malloc(sizeof(struct person_array));

has two problems. You've already allocated train_array[i]; what you need to allocate is space for the start and destination elements within each train_array[i]. Secondly, you haven't defined person_array anywhere in your code, which is why the compiler is complaining about a non-existent type.

You don't need to loop through the file twice; you can extend your array as you go as I demonstrated above. Here's an example of how you could restructure your code:

int done = 0;
...
size_t rows = 0;
size_t totalRows = 16;  // initial array size
train_array = malloc( sizeof *train_array * totalRows ); // initial allocation

while ( scanf( "%s %s %d %d", temp_station, 
                              temp_destination, 
                              &time_depature, 
                              &time_arrival) == 4 )
{
  if ( rows == totalRows )
  {
    struct train *tmp = realloc( sizeof *train_array * ( 2 * totalRows ));
    if ( tmp )
    {
      train_array = tmp;
      totalRows *= 2;
    }
    else
    {
      fprintf( stderr, "could not extend train_array past %zu elements\n", totalRows);
      break;
    }
  }
  train_array[rows].start_time = time_departure;
  train_array[rows].arrival_time = time_arrival;
  train_array[rows].start = malloc( strlen( temp_station ) + 1 );
  if ( train_array[rows].start )
    strcpy( train_array[rows].start, temp_station );

  train_array[rows].end = malloc( strlen( temp_destination ) + 1 );
  if ( train_array[rows].end )
    strcpy( train_array[rows].destination, temp_destination );
}

Note that when you're done with this memory, you'd deallocate it as follows:

for ( size_t i = 0; i < totalRows; i++ )
{
  free( train_array[i].start );
  free( train_array[i].end );
}
free( train_array );

---

<sup>1. In C, values of type void * can be assigned to any other pointer type without a cast. This is not true in C++; under a C++ compiler a cast would be required, but if you're writing C++ you should be using new and delete instead of malloc and free.
2. sizeof is an operator, not a function; you only need to use parentheses if the operand is a type name like int or float *.</sup>

Answer 2

1. You should call rewind(times); before the second do {} while(); to set the file position to its begin.

2. This

   train_array = (struct train*)malloc(sizeof(struct train_array)*rows);
   

   is wrong, because you does not have a struct train_array, change it to

   train_array = (struct train*)malloc(sizeof(*train_array)*rows);
   

   or

   train_array = (struct train*)malloc(sizeof(struct train)*rows);
   

3. This

   train_array[i] = (struct train*)malloc(sizeof(struct person_array));
   

   is wrong, because train_array[i] is a struct train, why do you try to assign a struct train * to it? This line should be deleted.

Answer 3

The first error looks to be because you're dereferencing your pointer to a struct type before the assignment. Also, you're allocating a "person_array" on that line, but type casting it to a "train_array" pointer, which I don't think is what you want to do.

Maybe this is what you meant to do?

train_array[i].start = (char*)malloc(sizeof(struct person_array));

The second error looks like you didn't declare the person_array struct. The code you posted doesn't have a definition, so if you don't have it defined elsewhere, that's probably the problem.

Along the same lines, I don't think your first malloc is doing what you want it to do either. Maybe try this:

train_array = (struct train*)malloc(sizeof(struct train)*rows);

"train_array" is a pointer, so it will only be size_t bytes. "struct train" is your data type that has the number of bytes you want to allocate, I think.