Cannot re-iterate a pointer array

Question

I have a pointer array of items that I read in from a file. These items are typedef structs.

My array is resizable as the file contains a lot of items.

while (fscanf(fp, "%X %[^\n]", &code, string) != EOF) {
    if (list->length >= list->capacity) {
        list->capacity *= 2;
        list->items = (Item**)realloc(list->items,
            list->capacity * sizeof(Item*));
    }
    item.code = code;
    strcpy(item.string, string);
    list->items[list->length++] = &item;
}

Assume list and item are declared already. Space is malloced for list.

static List * list; & Item item = { 0, { 0 } };

are declared and initialized earlier in the code. list is a global variable by necessity.

If I try to iterate this completed list by attempting to get the first value for instance:

printf("line %d: %X\t%-89s\n", 1, list->items[0]->code, list->items[0]->string);

I will get the last element in the file from this. However if I were to include this printf in the aforementioned while loop, I will get desired console output (when the index is list->length).

Why would I be experiencing this type of behavior? Is there something I am forgetting about traversing pointers?

Never `realloc` directly into the target pointer - always use a temp and check for NULL — Support Ukraine, Nov 10 '17 at 06:23
You're assigning the pointer to the **same** item all the time! Aren't you supposed to `malloc` a *new* `Item` too for every iteration. If you wear red, green and yellow shirt in succession there aren't 3 people with colour rainbow, there is just you wearing a yellow shirt. — Antti Haapala -- Слава Україні, Nov 10 '17 at 06:26
You need to show how `List` and `Item` are declared. It seems you are messing up between `Item` and `Item*` — Support Ukraine, Nov 10 '17 at 06:28
@AnttiHaapala I am sorry but your analogy does not make sense. I get what you are saying with the repeating thing though. — compolo, Nov 10 '17 at 07:48
@compolo but it absolutely does! You do not have n distinct `Item` objects. Just one, and `n` pointers pointing to **that one item**. — Antti Haapala -- Слава Україні, Nov 10 '17 at 07:50

Support Ukraine · Answer 1 · 2017-11-10T06:57:17.250

1

It seems that you use item as an automatic Item object and therefore fills the list->items array with pointers to the same Item object.

You probably want a new Item in each loop. Like:

Item* newItem;
while (fscanf(fp, "%X %[^\n]", &code, string) != EOF) {
    newItem = malloc(sizeof *newItem);
    if (!newItem) exit(1);

    if (list->length >= list->capacity) {
        list->capacity *= 2;
        list->items = (Item**)realloc(list->items,   // BTW DONT DO THIS - SEE BELOW
            list->capacity * sizeof(Item*));
    }
    newItem->code = code;
    strcpy(newItem->string, string);
    list->items[list->length++] = newItem;
}

BTW: Always realloc to a tmp pointer like:

tmp = realloc(....);
if (tmp == NULL)
{
    // error handling
}
else
{
    list = tmp;
}

Further: In C it is normally not recommended to cast malloc/ realloc / calloc. It is also typical to use sizeof *pointer instead of sizeof(type)

So instead of:

list->items = (Item**)realloc(list->items, list->capacity * sizeof(Item*));

you could do:

tmp = realloc(list->items, list->capacity * sizeof *tmp);

where tmp is the same type as list->items

edited Nov 10 '17 at 06:57

answered Nov 10 '17 at 06:39

Support Ukraine

42,271
4
38
63

Also drop a note about not casting the return of `realloc`. See: [**Do I cast the result of malloc?**](http://stackoverflow.com/q/605845/995714) (the same applies to `realloc` and `calloc`) – David C. Rankin Nov 10 '17 at 06:45
@DavidC.Rankin I already had a comment about that but now I have tried to make it a bit more clear. – Support Ukraine Nov 10 '17 at 06:57
How then would I go about freeing my temp? – compolo Nov 10 '17 at 07:43
@4386427 gotcha. I understood the original to be a copy/paste issue, but it is always good to make sure you are not repeating those practices in the answer. Good fix. – David C. Rankin Nov 10 '17 at 07:46
1

@compolo You never have to `free (temp)`. On successful `realloc`, `temp` is assigned to your original pointer and the block will be freed when you free the original pointer. On failure to `realloc`, `NULL` is returned and there is nothing allocated to `temp`, the original pointer is preserved and still points to the original block that existed prior to the `realloc` call (which is why you never `realloc` the original -- if it fails `NULL` is returned and you lose your reference to your original block of memory) – David C. Rankin Nov 10 '17 at 07:49
If it is practice not to cast realloc, malloc or calloc, what do I cast? I was taught to cast malloc especially. – compolo Nov 10 '17 at 07:50
Can you provide an example in the first code block of how you would implement this temp functionality? – compolo Nov 10 '17 at 07:51

Cannot re-iterate a pointer array

1 Answers1