[Solved] Push pop stack operation in C [closed]

Question

Here you have again some things wrong. Before you create new threads with
similar content, stick to one thread.

In your code you also never check if malloc fails. It’s always better to do that. For simplicity I’ve omitted these checks in my suggestions.

1.
Why do you do S1[0].top=-1; in makeEmpty? create already does that and writing in that way (instead of S1->top = -1) makes the code hard to interpret, because it could mean that you are treating the result of create as an array of Stacks. It’s not wrong, it makes harder for us to interpret your intentions.

Stack makeEmpty(void)
{
    Stack S1=create(100);
    return S1;
}

is enough.

2.
In push and pop you have to check first if the operations are valid. That
means:

for push: check that you still have space to do the operation
for pop: check that you have at least one element on the stack.

The code could be:

int isFull(Stack S)
{
    return (S->size - 1 == S->top);
}

int push(char X, Stack S)
{
    if(isFull(S))
        return 0;

    S->array[++S->top]=X;
    return 1;
}

int pop(Stack S, char *val)
{
    if(isEmpty(S))
        return 0;

    *val = S->array[S->top--];
    return 1;
}

I changed the signatures of these functions to let the user know if the
operation were successful. Otherwise you have no idea and you end up with
undefined values. In main your should change the pop calls as well:

char el;
pop(S1, &el);

3.
What do you want deleteStack to do: reset the stack or free the memory? You
are mixing those in a bad way.

If you want to reset (meaning to pop all elements at once), then all you have
to do is set top = -1. You don’t have to free array. If you do, then you
need to reallocate memory for array again.

When using free you can only pass to free the same pointer
you’ve got from malloc/calloc/realloc. In your code you are passing a
8 bit integer as a pointer address and freeing that, that will make your
program crash 100% of the time.

void deleteStack(Stack S)
{
    S->top = -1;
}

If you want to free the memory

void freeStack(Stack S)
{
    free(S->array);
    free(S);
}

but that also means that you cannot access the stack any more. Note that I
changed the name of the function to make it more clear its intention.

If you use my suggestions, you have to change them in main as well,
specially the pops.

EDIT:
Sebivor’s quote from the comments:

Don’t hide levels of pointer indirection behind typedef

Yes, that also makes the code hard to read. In my suggestions I didn’t change that, but I definitely would change

typedef struct stack Stack;

and in every function that has Stack S as argument change it to Stack *S. (See also Is it a good idea to typedef pointers — the short answer is ‘No’.)

Accepted Answer