delete[] doesnt work

i am tryinh to create stack using linked list.
function below is to pop the last element and my problem is that the previous ptr is the same as current ptr though i did delete it after returning the string to the function.

where did i do wrong?

Code:

---


std::string Stack::pop()
{ 
       
        node *ptr, *ptr2 = NULL;

                //empty
        if (start == NULL ){
                return "";
        }

        //more items
        for (ptr = start ; ptr->next != NULL ;ptr=ptr->next){
                ptr2=ptr;
                cout << "ptr2: " << ptr2->data << endl;
                cout << "ptr: " << ptr->data << endl;
        }
 
        std::string ans = ptr->data;
               

        return ans;
        delete ptr;

}

---

thanks.

Quote:

---

Originally Posted by effa

i am tryinh to create stack using linked list.
function below is to pop the last element and my problem is that the previous ptr is the same as current ptr though i did delete it after returning the string to the function.

where did i do wrong?

Code:

---


std::string Stack::pop()
{ 
       
        node *ptr, *ptr2 = NULL;

                //empty
        if (start == NULL ){
                return "";
        }

        //more items
        for (ptr = start ; ptr->next != NULL ;ptr=ptr->next){
                ptr2=ptr;
                cout << "ptr2: " << ptr2->data << endl;
                cout << "ptr: " << ptr->data << endl;
        }
 
        std::string ans = ptr->data;
               

        return ans;
        delete ptr;

}

---

thanks.

---

Maybe it's because you didn't listen to the warning that the compiler should have been screaming at you about an unreachable code path. How can it get to the delete ptr; statement if you have a return right before it?

but i tried to return after the delete.it didnt work because of memory leak.to think of that,it does makes because if i did that means im trying to something that doesnt exist..correct me if im wrong...

plus,i didnt receive any errors or warnings....

If you're using Visual C++, change to Warning Level 4 in the project settings. If you're using gcc, use the -Wall compiler flag.

I think I'll need to see more code to figure out what's wrong. Either that or I need more sleep.

hope anyone here can point out where my mistake was.tyvm. :)

h. file

Code:

---

class node {

        public:
                          std::string data;
                          node *next;
};
                       

class Stack
{
    public:
                node *start, *last;
                unsigned long int i;

        Stack();
                void push(std::string n);
                std::string pop();
        unsigned long count();
        ~Stack();

};

---

cpp file

Code:

---


#include <string>
#include <iostream>
#include "stack.h"

using namespace std;

Stack::Stack()
{
       
    start =NULL;
        last=NULL;
        i =0 ;
       
       
}

void Stack::push(std::string n)
{
        node*ptr;
        ptr=new node;
        ptr->data = n;
        ptr->next = NULL;

        if(start == NULL){
               
                start = ptr;
                last = ptr;       
               
                }
        else{
                last->next = ptr;
                last = ptr;
                       
        }

}

std::string Stack::pop()
{ 
       
        node *ptr, *ptr2 = NULL;
    //empty
        if (start == NULL ){
                return "";
        }

        else{
        //more items
                for (ptr = start ; ptr->next != NULL ;ptr=ptr->next){
                        ptr2=ptr;
                        cout << "ptr2: " << ptr2->data << endl;
                        cout << "ptr: " << ptr->data << endl;
                }
 
                std::string ans = ptr->data;
               
                cout << "ptr2: " << ptr->data << endl;
               

                return ans;
               
        }
        delete ptr;

}

unsigned long Stack::count()
{
  node * ptr;
 
  for( ptr = start ; ptr != last ; ptr = ptr->next)
  {
          i++;
  }
       
  cout << i;
  return i;
}


Stack::~Stack()
{
  node *ptr;

  while( start != NULL )
  {
      ptr = start->next;
      delete start;
      start = ptr;
  }

}

//
//int main(){
//        Stack s;
//        s.push("HELLO");
//        s.push("WORLD");
//        s.push("WORLD");
//        s.push("WORLD");
//        s.push("WORLD");
//        s.pop();
//        s.pop();
//        s.pop();
//}

---

I think the pop works better when the value is right there, instead of on the other end of the list. In other words, O(1), rather than an O(n) search. Since it's a stack, you know the value's always immediately available:

Code:

---

#ifndef STACK_H
#define STACK_H

class node {

public:
    std::string data;
    node *next;
};
                       
class Stack
{
public:
    node *last;
    unsigned long int i;

    Stack();
    void push(std::string n);
    std::string pop();
    unsigned long count();
    ~Stack();
};

#endif // STACK_H

---

Code:

---

#include "stdafx.h"

#include "Stack.h"

using namespace std;

Stack::Stack()
{
  last=NULL;
  i =0 ;
}

void Stack::push(std::string n)
{
  node*ptr;
  ptr=new node;
  ptr->data = n;
  ptr->next = NULL;

  if( last == NULL )
  {
    /* It's empty */
    last = ptr;
  }
  else
  {
    /* Not empty */

    /* Store the current last. */
    node *prevLast = last;

    /* This is the new last */
    last = ptr;

    /* The next-to-last */
    last->next = prevLast;
  }
}

std::string Stack::pop()
{ 
  node *ptr, *ptr2 = NULL;

  if( last == NULL )
  {
    return "";
  }

  std::string ans = last->data;

  ptr = last->next;
  delete last;
  last = ptr;

  return ans;
}

unsigned long Stack::count()
{
  /*
  node * ptr;
 
  for( ptr = start ; ptr != last ; ptr = ptr->next)
  {
    i++;
  }
       
  cout << i;
  return i;
  */
  return 0;
}


Stack::~Stack()
{
  node *ptr;

  while( last != NULL )
  {
    cout << "Cleaning up " << last->data << endl;
      ptr = last->next;
      delete last;
      last = ptr;
  }
}

//
//int main(){
//        Stack s;
//        s.push("HELLO");
//        s.push("WORLD");
//        s.push("WORLD");
//        s.push("WORLD");
//        s.push("WORLD");
//        s.pop();
//        s.pop();
//        s.pop();
//}

---

Quote:

---

Originally Posted by effa

hope anyone here can point out where my mistake was.tyvm. :)

---

Your main mistake is that you didn't tell us what was wrong with the code you had. You should also politely ask a question rather than just tossing the code on the floor and hoping someone will take care of everything. We are not your mother and do not do your laundry for you!

Quote:

---

Code:

---

std::string Stack::pop()
{ 
       
        node *ptr, *ptr2 = NULL;
    //empty
        if (start == NULL ){
                return "";
        }

        else{
        //more items
                for (ptr = start ; ptr->next != NULL ;ptr=ptr->next){
                        ptr2=ptr;
                        cout << "ptr2: " << ptr2->data << endl;
                        cout << "ptr: " << ptr->data << endl;
                }
 
                std::string ans = ptr->data;
               
                cout << "ptr2: " << ptr->data << endl;
               

                return ans;
               
        }
        delete ptr;

}

---

---

You don't understand what's going on there. You're still trying to do the delete after the return. Both the if and the else branch explicitly return and so never reach the end of the function.
You don't appear to have turned your warning level up, as was so rightfully suggested. Your compiler will tell you about mistakes like these if you let it. If neither of the options suggested apply to the compiler you are using, please tell us what compiler it is so that we may tell you how to allow your particular compiler to warn you.

Saying something like "delete doesn't work" is like saying "my sword doesn't work", for which the problem is that you're clearly holding it at the wrong end. "The pointy end goes in the other guy" ;)
Which incidentally is also the problem with your stack operations. Your Push and Pop functions must do their thing at the head end of the list.

goatless: As tempting as it is to do someone's homework for them when it's so easy for you, you're doing everyone a disservice by doing so.

You also never adjust the value of last.

As mentioned earlier, to implement a stack it is much easier to push and pop at the beginning of the singly linked list. What matters is that items that are added last come out first. As long as this holds, it doesn't matter whether the items are added to the end or to the beginning of the list.

Quote:

---

Originally Posted by iMalc

goatless: As tempting as it is to do someone's homework for them when it's so easy for you, you're doing everyone a disservice by doing so.

---

I'll have to plead no contest to that charge. :D I should have left it at reconsider getting the data from the beginning of the end.

I thought he stated the problem, though. He said last is the same as its previous pointer. My rough guess was that he wasn't cleaning up the last item correctly.

And also, the homework really isn't complete, though the core functionality is there. The class needs to handle the assignment operator and to implement counting.

I think I must repeat the age old tale:
Just because you get an error after you make something right, doesn't necessarily mean that it's wrong.
Moving the delete before the return is right. But it doesn't solve all problems. It merely solves one problem, but it is nevertheless right.

1)Count should not be that complicated. If you push, increment a count, if you pop, decrement it.

2)If you listened to me in your last thread, I warned about trying to return a value from pop(). Are you beginning to understand why?...

HINT: Implement a top() function. Make pop() return void (i.e. nothing)

3)Again, 2 pointers are not needed to implement a stack. Think about these two cases:

a) you have one item in the stack.

b) you have 0 items in the stack.

How can you test for these conditions?

Well....

Code:

---

// if this is roughly the node

struct Node{

  std::string str;
  Node* next;
};

// and this is your stack

class Stack{

  // ... stuff ...

  private:
 
    Node* head;
};

// Then you can do this:

if( head->next == 0 ) // We have one item in the stack

// and

if( head == 0 ) // We have 0 items in the stack

---

If you're having difficulty conceptualizing a "stack", think literally of a stack of plates. You put plates on the top, and you remove plates from the top. You wouldn't dare try and remove a plate from the bottom.

This means, we only care about the top item. While yes, it's necessary to "link" them all, there's no need to have a pointer to the "last" or "bottom" element, we simply don't care.

Now, special care needs to be taken when "popping" items off the stack (we need to know if there's more than one, one, or zero items) but again, if you move the pointers around correctly, you can use the above code to check for those conditions without necessitating the use of two Node pointers in your stack class.

Quote:

---

Originally Posted by goatless

And also, the homework really isn't complete, though the core functionality is there. The class needs to handle the assignment operator and to implement counting.

---

That's very much true. You gave some working correct code for some of it, but there was plenty left to do. In fact leaving the head called 'last' was a nice confusing touch for anyone who might take the code verbatim ;)

Job of raising awareness about homework done.