Help required

[anubhavjain346]

Hi All,
I am facing a issue with my stack code.
Please help me in identifying the error and rectifying it.

Here's my pop code:

int Stack::pop(NODE *head)
{
        cout<<"In pop"<<endl;
        if(head == NULL)
        {
                cout<<"No data in head available"<<endl;
                return (int)NULL;
        }
        else
        {

                NODE *temp=NULL;
                //Point the temp node to the linked list
                temp = head;
                //move head to the next node
                head = head->next;
                int num = temp->data;
                //make sure temp is not pointing to next element so that we can      free it
                temp->next = NULL;
                delete temp;
                return num;
        }
}

Now the issue is, when I call pop for the first time, item is popped but when I call the pop for second time, a glitch is detected.



Thanks in advance

[HiteshVaghani1]

If you are using linklist than your previous node should be the top of the stack not next... it's LIFO..

[viliml]

First, you don't need to initialize temp to NULL if you are gonna assign something else to it in the very next operation, so instead of

NODE *temp=NULL;
                //Point the temp node to the linked list
                temp = head;

make NODE *temp=head
Also, when you return (int)NULL, it shows you don't know what you're doing, becouse somewhere in the compiler, there is a #define NULL 0 !

[anubhavjain346]

@Hitesh

Suppose I have a list.

5-> 4-> 3-> 2-> 1->NULL

In this 5 is the top of the stack.
Now I want to pop, so 5 should be popped annd 4 should bbe the new top of stack.

so isn't the above code doing the same??

@viliml

I thought its always a good practice to take a initial pointer as NULL to avoid any dangling pointers or other error.

Secondly, if we donot typecast NULL to int, it will give compilation error. Please check.

[viliml]

Yes, the algorithm is wrong too! You don't erase the head, but the tail! What you made is a queue! with a list 5-> 4-> 3-> 2-> 1->NULL, 1 should be erased, becouse it is the last item put in!

[viliml]

Also, for the compilation error, try this:

#include <iostream>
#include <cstdlib>
#include <typeinfo>

using namespace std;

int test()
{
    return NULL;
}

typedef decltype(NULL) NULLTp;

int main()
{
    test();
    cout<<"See?"<<endl;
    #if int==NULLTp
    cout<<"See?"<<endl;
    #endif
    if (typeid(int)==typeid(NULL)) cout<<"See?"<<endl;
    system ("pause");
}

[HiteshVaghani1]

@anubhavjain346
than it's not stack. it's queue about what you are talking.

[anubhavjain346]

@Hitesh...

well... isn't this the process of forming stack:

head = NULL

head = 1->NULL

head = 2->1->NULL

head = 3->2->1->NULL

head = 4->3->2->1->NULL

head = 5->4->3->2->1->NULL

and on popping out, the sequence would be 5 4 3 2 1


@viliml....

I have moved the head to new top of stack and deleted temp node.

Isn't that correct?

[Ikaron]

I thought its always a good practice to take a initial pointer as NULL to avoid any dangling pointers or other error.

It is but in your case you're able to set "temp" to a valid pointer on initialization, so you might as well. Basically you just don't want your pointer initialized to garbage.

[Warnis]

Nothing in the code you posted really stands out as being wrong, what kind of glitch are you getting?

[anubhavjain346]

I am getting the following error:

In push
In push
In push
In push
In push
In pop
Item popped:9In push
In pop
Item popped:1In pop
*** glibc detected *** ./stack: double free or corruption (fasttop): 0x090f7048 ***
======= Backtrace: =========
/lib/libc.so.6[0x23dee4]
/usr/lib/libstdc++.so.6(_ZdlPv+0x22)[0x5619fc2]
./stack[0x804884e]
./stack[0x804898d]
/lib/libc.so.6(__libc_start_main+0xe6)[0x1e2e16]
./stack[0x8048611]
======= Memory map: ========
001a8000-001c8000 r-xp 00000000 08:02 174777     /lib/ld-2.12.90.so
001c8000-001c9000 r--p 0001f000 08:02 174777     /lib/ld-2.12.90.so
001c9000-001ca000 rw-p 00020000 08:02 174777     /lib/ld-2.12.90.so
001cc000-00359000 r-xp 00000000 08:02 174778     /lib/libc-2.12.90.so
00359000-0035b000 r--p 0018c000 08:02 174778     /lib/libc-2.12.90.so
0035b000-0035c000 rw-p 0018e000 08:02 174778     /lib/libc-2.12.90.so
0035c000-0035f000 rw-p 00000000 00:00 0 
003a9000-003d1000 r-xp 00000000 08:02 184772     /lib/libm-2.12.90.so
003d1000-003d2000 r--p 00027000 08:02 184772     /lib/libm-2.12.90.so
003d2000-003d3000 rw-p 00028000 08:02 184772     /lib/libm-2.12.90.so
00506000-00522000 r-xp 00000000 08:02 184766     /lib/libgcc_s-4.5.1-20100924.so.1
00522000-00523000 rw-p 0001b000 08:02 184766     /lib/libgcc_s-4.5.1-20100924.so.1
0081a000-0081b000 r-xp 00000000 00:00 0          [vdso]
0556c000-0564f000 r-xp 00000000 08:02 166300     /usr/lib/libstdc++.so.6.0.14
0564f000-05653000 r--p 000e2000 08:02 166300     /usr/lib/libstdc++.so.6.0.14
05653000-05655000 rw-p 000e6000 08:02 166300     /usr/lib/libstdc++.so.6.0.14
05655000-0565b000 rw-p 00000000 00:00 0 
08048000-08049000 r-xp 00000000 08:05 3411163    /home/anubhav/prac/tannenbaum/stack/stack
08049000-0804a000 rw-p 00000000 08:05 3411163    /home/anubhav/prac/tannenbaum/stack/stack
090f7000-09118000 rw-p 00000000 00:00 0          [heap]
b7769000-b776c000 rw-p 00000000 00:00 0 
b777a000-b777c000 rw-p 00000000 00:00 0 
bfa56000-bfa77000 rw-p 00000000 00:00 0          [stack]
Aborted (core dumped)

[Warnis]

I notice you're sending the head in to the function, is it changed to be head->next after the first pop call? Usually the internal structures of data structures are hidden to users, but in your case it looks like it's visible.

I would recommend having NODE* head as a private variable inside the class and not having any arguments when calling pop

[anubhavjain346]

wow.........thanks warnis.......

thanks for making my day.........i needed to use a double pointer for passing head to pop function so that the new head in main function could be updated.

The new code is as follows:

int Stack::pop(NODE **head)
{
        cout<<"In pop"<<endl;
        if(*head == NULL)
        {
                cout<<"No data in head available"<<endl;
                return (int)NULL;
        }
        else
        {

                NODE *temp=NULL;
                temp = *head;
                *head = (*head)->next;
                int num = temp->data;
                temp->next = NULL;
                delete temp;
                return num;
        }
}

Thanks again.