Stack with linked list, working, but I know I'm doing something dirty!

I finally got it working, but I'm not satisfied because I'm not freeing the memory myself. I'm not really sure what to do and was hoping to get a little advice.

Code:

---

#include <stdio.h>
#include <stdlib.h>

typedef struct list{
    char element;
    struct list * node;
} list;

int push(int data, list **ptr){
    list * tmp;
    tmp = malloc(sizeof(list));
    tmp->node = (*ptr)->node;
    tmp->element = data;
    (*ptr)->node = tmp;
    return 0;

}

int pop(list **ptr){
    if( (*ptr)->node != NULL ){
        int tmp;
        tmp = (*ptr)->node->element;
        free((*ptr)->node); //should be here I found out
        (*ptr)->node = (*ptr)->node->node;
        //free((*ptr)->node); causes junk data?
        return tmp;
    } else {
        fputs("Mismatched pop!\n",stderr);
        exit(1);
    }
}
   
int main(){
    list * head;
    head = malloc(sizeof(list));
   
    int i;
    for(i = 0;i<=10;i++)
        push(i,&head);
    for(i = 0;i<=10;i++)
        printf("%d\n",pop(&head));
    return 0;
}

---

Wow, I just figured it out, stupid mistake. Anyways, if anyone could at least check it to see if there are any other dirty tricks I pulled I'd be grateful.

Not sure what you did, but you could assign (*ptr)->node to a temporary pointer along with the data as you currently do. That way you don't lose track of it when you assign (*ptr)->node->node to it. Just before you return tmp, you free your temporary pointer (which has the old value of (*ptr)->node).

Freeing memory doesn't automatically zero it out, which is why you seem to be getting away with it when you do this:

Code:

---

        free((*ptr)->node); //should be here I found out
        (*ptr)->node = (*ptr)->node->node;

---

The last line there could in all likelihood crash your program, because you're dereferencing memory you've just chucked out.


Quzah.

Would this fix it?

Code:

---

char pop(list **ptr){
    if( (*ptr)->node != NULL ){
        char tmp;
        list * temp;
        temp = malloc(sizeof(list));
        tmp = (*ptr)->node->element;
        temp->node = (*ptr)->node->node;
        free((*ptr)->node);
        (*ptr)->node = temp->node;
        free(temp);
        return tmp;
    } else {
        printf("\nMismatched pop!\n");
        exit(0); //Not a program error.
    }
}

---

No, and you have created another memory leak, why do you call malloc? Leaving out some code here but this is the basic idea.

Code:

---

list *old = (*ptr)->node;
(*ptr)->node = (*ptr)->node->node;

free(old);
return tmp;

---

Code:

---


char pop(list **ptr){        if( (*ptr)->node != NULL ){                char tmp;                list * temp = (*ptr)->node;                tmp = (*ptr)->node->element;                (*ptr)->node = (*ptr)->node->node;                free(temp);                return tmp;        } else {                printf("\nMismatched pop!\n");                exit(0); //Not a program error..        }}

---

I'm not exactly sure how free(temp) is working, I think it frees what is pointed by the node pointed by the node of ptr...Right?

Edit: sorry, I've tried 4 or 5 times to get it to be multi-line, but the computer I was on just wasn't cooperating.

Instead: http://ideone.com/FlXGi

Only the pop function is changed.

free() works pretty much like this:

Code:

---

//some declarations and malloc
int *jim = malloc(20 * sizeof(int));
free(jim);
/* jim is dead now and cannot be used */

/*this is what you are doing */
jim[5] = 3;

---