Why doesn't the following code work?

Question

The following code attempts to create a new variable of type presence( a kind of structure) but when I run it through gdb, it shows that the variable has the same address in every iteration? Shouldn't a local variable get a different address when it is being redeclared?

presence* hashtable[MAX];
int i = 0;

printf("---------ENTERING DATA FOR THE HASHTABLE------------------\n");
while (i < MAX)
{

        presence item; /* Same address on every iteration */
        printf("Enter item id : ");
        scanf("%d",&item.id);

        printf("Enter item name : ");
        item.name = (char*)malloc(SIZE*sizeof(char));
        getchar();
        fgets(item.name,SIZE,stdin);

        putItem(hashtable,&item);
        i++;

}

When a item of type presence is put inside the hashtable, should I allocate the memory and then assign item or do it simply(refer the following code)

int putItem(presence* hashtable[], presence* item)
{
    int key = hash(item->id);
    hashtable[key] = (presence*)malloc(sizeof(presence)); /* Required or not? or do the below straight away?*/
    hashtable[key] = item;
}

score 2 · Accepted Answer · answered Sep 08 '13 at 16:10

For 1: You should have presence *item; pointer type variable, allocate it and use it to fill the data. Your current implementation uses same variable in each iteration.

while (i < MAX)
{

        presence *item; /* pointer */
        item = malloc(sizeof(*item));
        printf("Enter item id : ");
        scanf("%d",&item->id);

        printf("Enter item name : ");
        item->name = (char*)malloc(SIZE*sizeof(char));
        getchar();
        fgets(item->name,SIZE,stdin);

        putItem(hashtable,item);
        i++;

}

For 2: You don't need to allocate memory. The line

hashtable[key] = (presence*)malloc(sizeof(presence));

is not needed.

However make sure key is less than MAX which is size of hashtable. Also, you may want to handle clashes appropriately.

I would also add that local variables do not persist beyond their scope, which is also fixed by using a pointer and allocating memory. — BergQuester, Sep 08 '13 at 16:12

score 1 · Answer 2 · answered Sep 08 '13 at 16:09

Your local variables are volatile. When you create an instance, it disappears when the variable goes out of scope. So, you may get the same address in the following iteration.

In the seconds instance, you do need to malloc the space for presence and need to copy it using memcpy. Instead of using hashtable[key] = item, you should do memcpy ( hashtable[key], item, sizeof ( item ) );

score 1 · Answer 3 · answered Sep 08 '13 at 16:11

Local variables typically get only one address. As the variable item goes out of scope on the loop's back edge, the compiler is free to reuse its storage on the next execution of the loop. Either way, storing a pointer to a local variable to a data structure that lives longer than the scope of the local variable is a bad idea.
Depends on what you need: if the values in the hash table should be independent of the values that are passed in you should allocate memory in the function. If they shouldn't, then not.

score 1 · Answer 4 · edited May 23 '17 at 11:50

Shouldn't a local variable get a different address when it is being redeclared?

I don't see why it is a must (for any variable). In fact, it's quite a logical decision for a compiler to place it at the same memory location.

But anyway, the approach you are currently taking is wrong: you are using the address of a local (block-scope automatic) variable after it's out of scope, which is undefined behavior. You should allocate memory dynamically for the individual elements instead.

When a item of type presence is put inside the hashtable, should I allocate the memory and then assign item or do it simply

Does "simply" mean "without allocating memory"?

Apart from that, this completely depends on what the contract between different functions is. Also, allocate memory for what? In your particular case, these lines:

hashtable[key] = (presence*)malloc(sizeof(presence));
hashtable[key] = item;

would leak memory since you are overwriting the pointer to the allocated memory right after having allocated it. So no, don't do the memory allocation. But, if you hadn't declared the hash table (not the items inside it!) as

presence* hashtable[MAX];

(i. e. an array of pointers) but you had a pointer-to-pointers:

presence **hashtable;

you would have had to allocate memory for the hash table (outside the insertion function, obviously):

presence **hashtable = malloc(sizeof(*hashtable) * N_ITEMS);

Also, please do NOT cast the return value of malloc()!

Why doesn't the following code work?

4 Answers4