Printing a dynamic array in C

Question

I'm writing a program in C. I'm using a dynamic array in my program, and am using a for loop to cycle through the items in the array. The problem I'm having, is that when I go to print the list to the screen (in the for loop), all previous items in the list are changed to the most recently created item. I don't know what's causing this. I have gone through the code numerous times in GDB, and I still can't find what's wrong.

/* For Loop, Displays Weapon List */
for (i = 1; i < num_places; i++)
{
    printf("%d. \n",i);
    printf("Des: %s \n\n",weap_List[i].description);
}

/* Add function, adds a weapon to the list */
int Add_weap(weapon new_weap) 
{
    if (num_places == num_allocated) 
    {
        if (num_allocated == 0)
                num_allocated = 3;
        else 
            num_allocated *= 2;
        void *_tmp = realloc(weap_List, (num_allocated * sizeof(weapon)));
        weap_List = (weapon*)_tmp;
    }
    num_places++;
    weap_List[num_places] = new_weap;
    return num_places;
}

/* Code that invokes the function, adding the weapon to the list */
printf("Adding new weapon \n");
weapon temp;
printf("Please enter the description of this new weapon. \n");
scanf("%s",weap.description);
Add_weap(temp);

/* Weapon structure */
typedef struct {
    char* description;
} weapon;

If you could point me in the right direction, that would be much appreciated.

So, when you print the weapon list, it prints everything the same? — Seth Carnegie
– Seth Carnegie, Commented Apr 29, 2011 at 0:43
It would also be helpful to see the constructor for weapon. That, and you're skipping the 0th slot of your array. — Seth Carnegie
– Seth Carnegie, Commented Apr 29, 2011 at 0:50
Well, this is C, so there are no constructors. But the point is on target -- can you show us how you're allocating/initializing your weapon structs? — QuantumMechanic
– QuantumMechanic, Commented Apr 29, 2011 at 4:34

Richard Schneider · Accepted Answer · 2011-04-29 00:54:25Z

2

You increment num_places at the wrong time.

Change

  num_places++;
  weap_List[num_places] = new_weap;

to

  weap_List[num_places++] = new_weap;

answered Apr 29, 2011 at 0:54

Richard Schneider

35.5k9 gold badges59 silver badges74 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

sarnold Over a year ago

D'oh :) missed this. Nice catch.

QuantumMechanic Over a year ago

True, but that wouldn't address the OP's complaint that all previous elements are being changed to be the most recently entered weapon when he prints them out. sarnold -- could you post the definition of the weapon struct? I mention this because when I used a simple struct weapon { char description[40]; }; I did not have the OP's problem. I did have the issue you mentioned with the first one being skipped, but I did not have the OP's problem of all the printing being of the most recent one.

JC Leyba Over a year ago

Yeah, it works if I make it weapon a normal string. It doesn't work if I am doing it this way: char* description; I think I'm making a basic mistake with pointers, but I'm not sure.

QuantumMechanic Over a year ago

If your description field is char* description, where/how are you allocating the actual memory for the description?

sarnold · Accepted Answer · 2011-04-29 00:55:54Z

Some quick thoughts:

for (i = 1; i < num_places; i++)
{

num_places, being an array index, will start at 0. This loop will fail badly with only zero or one weapons in your list. (The condition i < num_places is checked after the first iteration of the loop body. If weap_List has one element (at index 0), this loop will access memory that is not allocated and not print the weapon. Start array-handling loops at 0. The only times you should ever tolerate one-indexed arrays is when moving a huge amount of FORTRAN code to C; when starting from scratch, use zero-indexed arrays, as K&R intended. :)

   void *_tmp = realloc(weap_List, (num_allocated * sizeof(weapon)));
   weap_List = (weapon*)_tmp;

If _tmp were declared (weapon *) tmp = realloc(...), then you wouldn't need the (weapon *) cast on the next line.

Tiny nitpick: doubling the size is pretty expensive way to go when adding the 196609th weapon. Consider just growing by eight each time. (I'm guessing from your names that you're probably not going to be adding one item per second for weeks on end.)

Tiny nitpick: Avoid _prefix variable names, they are reserved for the C runtime environment. Yes, just about every program uses them, and yes, I've used them, and yes this one is probably safe :) but they are technically reserved. A plain tmp would suffice.

Larger nitpick: you're not checking the return value from realloc(3). It can fail. Your application should handle failure as gracefully as possible.

Collectives™ on Stack Overflow

Printing a dynamic array in C

2 Answers 2

4 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related