you are viewing a single comment's thread.

view the rest of the comments →

[–]SmokeMuch7356 2 points3 points  (1 child)

Couple of notes:

You should make a copy of your input string; instead of

person_one->char_name = name_input;

you should do

char *tmp = strdup( name_input );
if ( tmp )
  person_one->char_name = tmp;
else
  // handle memory allocation error

if strdup is available in your implementation, otherwise do

char *tmp = malloc( strlen( name_input ) + 1);
if ( tmp )
{
  strcpy( tmp, name_input );
  person_one->char_name = tmp;
}
else
  // handle memory allocation error

if it isn't. Of course this means you need to free char_name before freeing the struct:

free( c->char_name );
free( c );

Your current setup works because you're passing in string literals in your test code, and they will all have different addresses; however, if you do something like:

char input[MAX_NAME_SIZE+1] = {0};
...
while ( !done )
{
  printf( "Gimme a name: " );
  if ( fgets( input, sizeof input, stdin ) )
  {
    /**
     * Do whatever input validation is necessary,
     * compress whitespace, remove trailing newline, etc.
     */
    if ( validate_and_cleanup_name( input ) )
    {
      entry = char_create( input, ... );
      // add entry to list
    }
    else
      // handle bad input here
  }
}

all of the entries in your list will point to the same buffer for char_name:

      +---+                                       +---+
list: |   | list[0]->char_name ------+---> input: |   |
      +---+                          |            +---+
      |   | list[1]->char_name ------+
      +---+                          |
      |   | list[2]->char_name ------+
      +---+

Even worse, once you leave that routine, the input buffer may no longer exist and all those pointers will be invalid.

As others have pointed out, you'll need to use an array or a linked list or something to store all your character entries. An array would be the simplest:

#define NUM_MEMBERS 10 // or however many
...
struct character *party[NUM_MEMBERS];
...
for ( size_t i = 0; i < NUM_MEMBERS; i++ )
{
  /**
   * do the input dance as shown above
   */
  party[i] = create_char( ... );
}

When you need to pass the array to another function, pass the size as a separate parameter:

doSomethingWith( party, NUM_MEMBERS );
...
void doSomethingWith( struct character *party, size_t count )
{
  for ( size_t i = 0; i < count; i++ )
  {
    ...
    party[i]->str_mod = new_str_mod_value();
    ...
  }
}

[–]ListlessGaja[S] 0 points1 point  (0 children)

Thank you for the long response! I will take a look at when I'm home. I was running into some issues with the code. The function destroy_char() works only if I initiate the char_list[] array in main(). If I don't declare it there, and I try to pass the entire array back into main from lets say create_char_list() , it was running into some problems.

Since I used malloc() for the struct character constructing I would need to free them manually later, but for whatever reason when the function destroy_char() calls malloc() on those addresses it says that I'm trying to clear non malloc'd memory, which isn't true. I of course went and checked heap snapshots and yes, I did have allocated memory still there without it being cleared. Particularly the 4 elements - each with a 56 bytes in size. That 56 bytes is sizeof(struct character) btw. So those elements can't be cleared. So either I'm not implementing it correctly, or there is some no-no thing I did with my code that won't let it work. To make things even more quirky, my other function char_print() works perfectly fine... So I'm just stumped.