Arrays of structures, dynamic distribution

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


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;

            /*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);

        } while (file != EOF);

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

        /*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);
    } while (file != EOF);

    /*closes the file*/

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 );


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 1 , 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) 2 . 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,
                              &time_arrival) == 4 )
  if ( rows == totalRows )
    struct train *tmp = realloc( sizeof *train_array * ( 2 * totalRows ));
    if ( tmp )
      train_array = tmp;
      totalRows *= 2;
      fprintf( stderr, "could not extend train_array past %zu elements\n", totalRows);
  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 );

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 * .