Need a little help with printing a stack

Hello, I've been learning stack ADT from one of the books and I've written the following stack programs which works fine if the "print(stack)" is called after "popStack(stack)."

However if I call "print(stack)" before "popStack(stack)", something goes wrong. Could you please help, below is the code for the same.

Thanks in advance.

Code:

#include<stdio.h>
#include<conio.h>
#include<stdlib.h>
typedef struct node
{
        void *dataPtr;
        struct node *link;
}STACK_NODE;
typedef struct 
{
        int count;
        STACK_NODE *top;
}STACK;
STACK *createStack(void)
{
      STACK *stack;
      stack = (STACK *)malloc(sizeof(STACK));
      if(stack)
      {
               printf("Stack created successfully. . .");
               stack->count = 0;
               stack->top = NULL;
      }
      return stack;
}
void pushStack(STACK *stack, void *dataInPtr)
{
     STACK_NODE *newNode;
     if(newNode = (STACK_NODE *)malloc(sizeof(STACK_NODE)))
     {
                printf("\n\n%d pushed onto Stack successfully.", (int *)dataInPtr);
                newNode->dataPtr = dataInPtr;
                newNode->link = stack->top;
                stack->top = newNode;
                (stack->count)++;
     }
}
void print(STACK *stack)
{
     STACK *temp;
     temp = stack;
     printf("\n\n");
     while(temp->top != NULL)
     {
                 printf("%d\n", temp->top->dataPtr);
                 temp->top = temp->top->link;
     }
     free(temp);
}     
void *popStack(STACK *stack)
{
     STACK_NODE *temp;
     void *dataOutPtr;
     if(stack->count == 0)
                     dataOutPtr = NULL;
     else
     {
         temp = stack->top;
         dataOutPtr = stack->top->dataPtr;
         stack->top = stack->top->link;
         (stack->count)--;
         free(temp);
     }
     return dataOutPtr;
}
int main(void)
{
    STACK *stack;
    stack = createStack();
    pushStack(stack, 10);
    pushStack(stack, 20);
    //print(stack);
    printf("\n\nElement popped is %d", (int *)popStack(stack));
    print(stack);
    getch();
    return 0;
}

Your print function modifies the stack. Worst off all it frees the stack (but not the nodes) passed to it. Accessing the stack after that results in undefined behavior because the memory is no longer allocated to your program.

@christop - That's exactly what I thought. But I know no other way to print the stack. Any help on this one?

Can someone else help me with this?

christop told you already that you shouldn't modify your stack.

Hint: What other type "temp" could be instead of "STACK *"?

Bye, Andreas

I've just noticed that you have another problem with your code:

Code:

$ make foo
cc -Wall -Wextra -ggdb3    foo.c   -o foo
foo.c: In function ‘pushStack’:
foo.c:28:6: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
foo.c:30:17: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘int *’ [-Wformat]
foo.c: In function ‘print’:
foo.c:44:18: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘void *’ [-Wformat]
foo.c: In function ‘main’:
foo.c:69:5: warning: passing argument 2 of ‘pushStack’ makes pointer from integer without a cast [enabled by default]
foo.c:25:6: note: expected ‘void *’ but argument is of type ‘int’
foo.c:70:5: warning: passing argument 2 of ‘pushStack’ makes pointer from integer without a cast [enabled by default]
foo.c:25:6: note: expected ‘void *’ but argument is of type ‘int’
foo.c:72:5: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘int *’ [-Wformat]

Code:

pushStack(stack, 10);
pushStack(stack, 20);

These two lines push memory addresses (0xa and 0x14) on the stack because the second parameter of pushStack() is a pointer to void.

Bye, Andreas

why don't you use a recursive function to print elements?
It became easier and you can do it without modify the stack.

Andreas and root: Thanks for your comments: I modified it and now it's working fine. Below is the code:

Code:

void print(STACK_NODE *temp)
{
     if(temp == NULL)
     {
             return;
     }
     printf("%d\n", temp->dataPtr);
     print(temp->link);     
}     
int main(void)
{
    STACK *stack;
    stack = createStack();
    pushStack(stack, 10);
    pushStack(stack, 20);
    printf("\n\n");
    print(stack->top);
    printf("\n\nElement popped is %d", (int *)popStack(stack));
    printf("\n\n");
    print(stack->top);
    getch();
    return 0;
}

Please have a look at it and let me know if it looks good now.

And Andreas, the second comment regarding "pushStack(stack, 10)" is correct. The compiler gives me a warning for this. But I don't have much idea on how to resolve this.

I'm not sure that your code is correct, in a stack you can't access to an element without have done the pop of the previous elements.
your print function scans the stack like a linked list.

Originally Posted by Shyam Raj

And Andreas, the second comment regarding "pushStack(stack, 10)" is correct. The compiler gives me a warning for this. But I don't have much idea on how to resolve this.

Your data element is a void pointer, thus you can only push pointers onto your stack. You can't push a number like 10 directly.

So why are you using a void pointer?

Bye, Andreas

@root - you said try using a recursive function to print the stack, and I think I've done it that way. How else can the stack be printed?

@Andreas - By specifying a void pointer, we can push an element of any type isn't it? Like a char/int/float. The program needn't be changed everytime. That was the idea behind this. Is it wrong?

Originally Posted by Shyam Raj

@root - you said try using a recursive function to print the stack, and I think I've done it that way. How else can the stack be printed?

Before saying "how" it's also important understand "why" I've said that.
So,now you have an ADT stack with just a print function, suppose that you want to add functions to find max and min, to add and multiplicate elements, to check a particular properties of the stack(if they are in decreasing order for example) and every function that can spot in your mind.
You can write functions that access directly to the implementation of the Data structures, it should work, but suppose that a day you wake up and say "why don't I use an array to represent the stack?", and now? You have to edit all functions you've defined previously, in this case probably you have to edit few things for every function, but it's a good thing begin thinking in this way: you define the basic functions that work directly with the implementation of the ADT, in a stack case probably they are: initialize, pop, push, top and free_stack, now they're in a "black box", for every other function you're gonna use them without know how they work and how the ADT is implemented(with list, arrays, etc).
In this way if you want to change your way to rapresent the Data Structure you'have just to edit the "black box" and not all.
Another good thing is to divide your code, a stack.h header file with declarations of basic functions,a stack.c file with definitions of the functions declared in stack.h, a "itemtype.h" header file with the implementation of the stack(in your case you define a type), and an "item.h" header file with the type you consider(int in your case).
Now you'll put all the other function(like your print function) in a "user.c" file.
Imo this is a good way to organize programs and to implement ADT.


After the why, it's the "how" time, try to think how may a recursive function print a stack using pop and push functions, I invite you to try, because if you solved this, then you can write every other function that works with an ADT stack because they are equal and change practically nothing between them.
It's a very short code, if you have problems just say
I hope to have been understood, and if I've written something wrong please correct me.

Originally Posted by Shyam Raj

@Andreas - By specifying a void pointer, we can push an element of any type isn't it? Like a char/int/float. The program needn't be changed everytime. That was the idea behind this. Is it wrong?

Ok, I understand your idea. But the way you do it, doesn't work.

The first problem is, that you can't use constants like 10 or 3.1415. You can only use a pointer to a variable which contains these values.

The second problem is, when you mix the elements on the stack (for example first push an integer pointer, then a double pointer, then a char pointer, ...) you can't determine the type of the element w hen you pop it. Since you only get back a void pointer, there is no way to know to which data type this pointer points to. But you need this information, because to be able to use the data later, you have to cast the void pointer to the appropriate type before dereferencing it to get the value.

I've played around with a stack implementation which uses a special data structure containing an enumeration element (describing the type of the data) and a union element (holding the actual data). It works with mixing integers, chars, strings, doubles and a custom struct (containing a string, an integer and a double) on the stack. But you have to know beforehand which data types you want to use.

Perhaps someone else has another/better idea.

Bye, Andreas

@root: Hello root, thanks for your detailed reply. Regarding the array implementation of stack, I do have an idea that stacks can be implemented that way - however I haven't tried it yet bcoz I want to master the ADT way before I try that one. Sure I'll implement it the other way as well.

The idea of having all basic functions in a header file and then just calling me is absolutely correct. I did it and its working fine.

Regarding how to do it, from your explanation I understand that you're saying - print an element every time you push it. And similarly print the element only you've popped it. Let me know if I've not got it right. If not, give me a small hint, I might me able to do it .

Thanks in advance.