Created function that pops the last value entered in the stack fails after second popping

Question

struct n {
    int data;
    struct n* next;

};
typedef struct n node;

node* push(node* s, int x) {
    node* temp;// temporary element
    temp = (node*)malloc(sizeof(node));
    if (temp == NULL) {

        printf("not enough memory.");
        return -1;
    }


   // creates a new element that stores the data and pointed by the root pointer
    temp->data = x;
    temp->next = s;
    s = temp;
    return s;

}
int pop(node* s) {
    if (s == NULL) {
        printf("Stack is empty\n");
        return -1;
    }
    // create a temporary elements points the root pointer.
    // after iteration temporary elements will be deleted.
    node* temp = (node*)malloc(sizeof(node));
    temp = s;
    int rvalue = temp->data;
    s = s->next;
    free(temp);
    return rvalue;

}

int main() {
    node* root = NULL;
    root = push(root, 10);
    root = push(root, 20);
    root = push(root, 30);

    printf("%d\n", pop(root));
    printf("%d\n", pop(root));
    return 0;
}

Push function creates a temporary struct element and stores the data that given in the function parameter. then temp's next pointer points the root pointer for linking list.

Pop function creates a temporary struct element that points the root pointer. then root pointer is iterated next last element that entered. And temporary element is deleted.

Trying to make the pop function returns the last entered elements of stack and remove it after every call, but it fails in the second. Beacuse in the pop function s* (root pointer) will be not iterated. How can I fix that? Thank you.

Answer 1

pass the node by reference not by value, for example:

int pop(node** s) {
    if (*s == NULL) {
        printf("Stack is empty\n");
        return -1;
    }
    node* temp = *s;
    int rvalue = temp->data;
    *s = (*s)->next;
    free(temp);
    return rvalue;
}

The call of pop would be like

printf("%d\n", pop(&root));

Answer 2

The function pop is invalid.

First of all it produces a memory leak because at first a memory was allocated and its address was assigned to the pointer temp and then the pointer was reassigned

node* temp = (node*)malloc(sizeof(node));
temp = s;

As a result in each call of the function there is allocated memory for a node that is not freed.

The second problem is that the pointer to the top node of the stack is passed to the function by value. So changing the corresponding parameter s that stores a copy of the value of the original pointer within the function

s = s->next;

does not change the original pointer itself to the top node of the stack.

Also pay attention to that the function should not issue any message. It is the caller of the function that decides whether to output a message. And the value -1 can be a valid data stored in the stack.

I would declare and define the function the following way

int pop( node **s, int *data ) 
{
    int success = *s != NULL;

    if ( success ) 
    {
        node *temp = *s;
        *data = temp->data;
     
        *s = ( *s )->next;

        free( temp );
    }

    return success;
}

and call the function like

int data;
if ( pop( &root, &data ) )
{
    printf("%d\n", data );
}
else
{
    puts( "Stack is empty" );
}

Using such an approach you can for example to obtain all values stored in the stack the following way

for ( int data; pop( &root, &data; )
{
        printf("%d\n", data );
}

There is also a drawback in the function push. It can return the value -1 as a pointer

if (temp == NULL) {

    printf("not enough memory.");
    return -1;
}

In this case such statements in main like that

root = push(root, 10);

can result in numerous memory leaks and undefined behavior when there is no enough memory to allocate a new node for the stack.

The function can look like

int push( node **s, int data ) 
{
    node *temp = malloc( sizeof( *temp ) );
    int success = temp != NULL;

    if ( success )
    {
        temp->data = data;
        temp->next = *s;
        *s = temp;
    }

    return success;
}

And the function is called at least like

push( &root, 10 );

That is the both above functions accept the pointer to the top node of the stack by reference indirectly through pointers to it.