You have a number of problems in your implementation of your stack. The first of which is in push()
, when you call:
temp->next = s->top;
s->top
is an uninitialized pointer whose value is indeterminate. That invokes Undefined Behavior. Since this occurs on your very first call to push(s, pu)
, you can have zero confidence in the operation of the remainder of your code.
Next, both your push
and pop
function fail to test and handle in an appropriate manner whether the node being pushed or popped is the first or last node in the stack. These must be handled differently as the handling of your prev
and next
pointers will depend on whether other nodes exist. (you can't assign a next
or prev
node if you are pushing the first node, and you can't assign a new top
on pop
if no more nodes exist.
(note: you also seem to have used prev
and next
in a somewhat inconsistent manner. As long as you use them consistently, it doesn't matter whether you use prev
for the existing top
on push
or next
. I tend to have prev
pointing down the stack and next
pointing from the bottom up - but you are free to do it the other way)
To properly handle the first and last node in your stack, you can do a simple check on isempty()
or simply check if (s->top == NULL)
or if (s->count == 0)
.
You also need to initialize all pointers and count
when you allocate storage for your stack and nodes. Failure to initialize all values on allocation will likely lead to an inadvertent attempted read from an uninitialized value (invoking additional instances of Undefined Behavior).
To eliminate that possibility, you can create simply helper functions, say create_node
and create_stack
to allocate, and validate the allocation, initialize the needed values, and then to provide a meaningful return indicating success or failure. You could do something simple like the following:
snode *create_node (int x)
{
snode *tmp = malloc (sizeof *tmp);
if (!tmp) {
perror ("create_node: memory exhausted.");
return NULL;
}
tmp->data = x;
tmp->prev = tmp->next = NULL;
return tmp;
}
stack *create_stack (void)
{
stack *tmp = malloc (sizeof *tmp);
if (!tmp) {
perror ("create_stack: memory exhausted.");
return NULL;
}
tmp->top = NULL;
tmp->count = 0;
return tmp;
}
Now there are no possibilities of an uninitialized value.
(Also note: while not an error, the standard coding style for C avoids the use of camelCase
or MixedCase
variable names in favor of all lower-case while reserving upper-case names for use with macros and constants. It is a matter of style -- so it is completely up to you, but failing to follow it can lead to the wrong first impression in some circles.)
With the helper functions defined, allocating and initializing your forward and reverse stacks in main()
becomes a simple matter of:
int pu;
stack *s = create_stack();
stack *rs = create_stack();
if (!s || !rs) /* validate the return from create_stack */
return 1;
Having created your forward and reverse stack, you can then handle the first and last node cases in push()
and pop()
with the addition of a simple conditional as described above:
void push (stack * s, int x)
{
snode *temp = create_node (x);
if (!s->top) /* is the stack empty? */
s->top = temp;
else {
s->top->next = temp;
temp->prev = s->top;
s->top = temp;
}
s->count++;
}
and for pop()
,
int pop (stack *s)
{
int x;
snode *temp;
if (isempty (s)) { /* checking empty as you did in original */
printf ("underflow");
return 0;
}
x = s->top->data;
temp = s->top;
if (s->top->prev)
s->top = s->top->prev;
s->top->next = NULL;
free (temp);
s->count--;
return x;
}
Adding a short prn_stack()
function to iterate over the stack without pop
ing and freeing the nodes, you could put together a short example program to test the push
, pop
and reverse
as follows:
#include <stdio.h>
#include <stdlib.h>
typedef struct node {
int data;
struct node *next, *prev;
} snode;
typedef struct {
snode *top;
int count;
} stack;
snode *create_node (int x)
{
snode *tmp = malloc (sizeof *tmp);
if (!tmp) {
perror ("create_node: memory exhausted.");
return NULL;
}
tmp->data = x;
tmp->prev = tmp->next = NULL;
return tmp;
}
stack *create_stack (void)
{
stack *tmp = malloc (sizeof *tmp);
if (!tmp) {
perror ("create_stack: memory exhausted.");
return NULL;
}
tmp->top = NULL;
tmp->count = 0;
return tmp;
}
int isempty (stack *s)
{
return (s->count == 0);
}
void prn_stack (stack *s)
{
snode *iter;
if (isempty(s)) {
puts ("stack empty");
return;
}
iter = s->top;
for (; iter; iter = iter->prev)
printf (" %d\n", iter->data);
}
void push (stack * s, int x)
{
snode *temp = create_node (x);
if (!s->top)
s->top = temp;
else {
s->top->next = temp;
temp->prev = s->top;
s->top = temp;
}
s->count++;
}
int pop (stack *s)
{
int x;
snode *temp;
if (isempty (s)) {
printf ("underflow");
return 0;
}
x = s->top->data;
temp = s->top;
if (s->top->prev)
s->top = s->top->prev;
s->top->next = NULL;
free (temp);
s->count--;
return x;
}
void reverse (stack * s, stack * rs)
{
while (!isempty (s))
push (rs, pop (s));
}
int main ()
{
int pu;
stack *s = create_stack();
stack *rs = create_stack();
if (!s || !rs)
return 1;
while (scanf (" %d", &pu) == 1)
push (s, pu);
printf ("stack:\n");
prn_stack (s);
reverse (s, rs);
printf ("\nreversed stack:\n");
while (!isempty (rs))
printf (" %d\n", pop (rs));
free (s);
free (rs);
return 0;
}
(note: do not forget to free
the memory you allocate for your forward and your reverse stacks after all the values have been popped, and always validate your memory use by using a memory-error checking program like valgrind
on Linux. There are similar programs for all OS's)
Example Use/Output
$ echo "10 9 8 7 6 5 4 3 2 1" | ./bin/stack_rev
stack:
1
2
3
4
5
6
7
8
9
10
reversed stack:
10
9
8
7
6
5
4
3
2
1
To validate your memory use and that you have freed all memory you allocate and that there are no memory errors, just run your code through the memory checker, similar to the following using valgrind
, e.g.
Memory Use/Error Check
$ echo "10 9 8 7 6 5 4 3 2 1" | valgrind ./bin/stack_rev
==18418== Memcheck, a memory error detector
==18418== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==18418== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==18418== Command: ./bin/stack_rev
==18418==
stack:
1
2
3
4
5
6
7
8
9
10
reversed stack:
10
9
8
7
6
5
4
3
2
1
==18418==
==18418== HEAP SUMMARY:
==18418== in use at exit: 0 bytes in 0 blocks
==18418== total heap usage: 22 allocs, 22 frees, 512 bytes allocated
==18418==
==18418== All heap blocks were freed -- no leaks are possible
==18418==
==18418== For counts of detected and suppressed errors, rerun with: -v
==18418== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Look things over and let me know if you have any further questions.