Push pop stack operation in C

Question

I tried to make a stack push pop operations with storing values from pop in an array. I have a few questions for this code.
1. Does the initialization part work well? When compiling, it seems there is no problem but I'm still curious.
2. Does the deletion part also work well? Although, I typed !, it seems it doesn't goes to else if(strcmp (str[i], "!")==0) part. 3. How could store values after pop operations? I want to store it as an array format but when I print out after storing them in the array, runtime error occured.
Here's the code:

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

typedef struct stack *Stack;

struct stack {
    char *array;
    int size;
    int top;
};//making stack


Stack create(int c);
Stack makeEmpty(void);//initialization
int isEmpty(Stack S)
{
    if(S->top==-1)
        return 0;
    else
        return 1;
};
void push(char X, Stack S)
{
    S->array[++S->top]=X;
};
char pop(Stack S)
{
    return S->array[S->top--];
};
void deleteStack(Stack S)
{
    while (S->top>=0)
    {
        free(S->array[S->top]);
        S->top--;
    }

};

int main (void)
{
    Stack S1=makeEmpty();
    char input[100], result[30], result1;
    char *word;
    int cnt=0,cnt2=0, temp=0, k=0, i,j,l;
    word=strtok(input, " ");
    char *str[20];

    while(1){
        if(fgets(input,100,stdin)){
            word=strtok(input, " ");//get input and tokenize
            cnt=0;
            while (word!=NULL)
            {
                str[cnt]=strdup(word);
                cnt++;
                word=strtok(NULL," ");  
            }       
        }

        for (i=0; i<cnt; i++)
        {
            if (strcmp(str[i],"(")==0 && (isEmpty(S1)==0))
            {
                push(str[i],S1);
                l++;
            }
            else if(strcmp(str[i],")")==0)
            {
                temp++;
                for(j=0; j<cnt2; j++)
                {
                    char result1=pop(S1);
                    result[k] =result1;
                    printf("%s ", result[k]);//This the place where I got runtime error
                    k++;
                }
                pop(S1);
                cnt2=0;     
            }
            else if(strcmp (str[i], "!")==0)
            {
                printf("in\n");
                deleteStack(S1);
                return 0;
            }

            else 
            {
                if  (isEmpty(S1)==1)
                {

                    if (l!=0)
                    {
                        push(str[i],S1);

                        if (strcmp(str[i],"(")!=0)
                        {
                            cnt2++;
                        }
                    }
                }
            }
        }
    }

    return 0;
}


Stack create(int c)
{
Stack S=(Stack)malloc(sizeof(struct stack));

    S->size=c;
    S->top=-1;
    S->array=(char *)malloc(sizeof(char)*c);

    return S;
}

Stack makeEmpty(void)
{
    Stack S1=create(100);
    S1[0].top=-1;
    return S1;
}

Answer 1

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'.)