Question about heap memory in class

[hopeman]

Hi all,

I have used Node* new() inside my List class. How can I delete it inside the Node class itself to avoid leakage.

#include <cstdlib>
#include <iostream>

using namespace std;

class List
{
	private:

		class Node
		{
			friend class List;
			private:
			        //Node()
				//{
				//	next = NULL;
				//	val = 0;
				//};
				Node(int Node_v)
				{
					next = NULL;
					val = Node_v;
				};
				~Node()
				{
					//next = NULL;
				};
				
				int val;
				Node* next;
		};

		Node* head;
		Node* current;

		//friend class has no access to static member!
		//static Node* head;
		//static Node* current;
	public:

		List(int v)
		{
			Node* newNode = new Node(v);
			if (head == NULL)
			{
				head = newNode; 
				current = newNode;
			        cout << " Data Structure is created and initialized. \n";
			}
			else
			{
			        cout << " Data Structure is already created. \n";
			};
			//delete newNode;
		};


		void Add(int v_add)
		{
			Node* newNode = new Node(v_add);
			current->next = newNode;
			current = newNode;
			//current->val=newNode->val;
			//delete newNode;
		};

		int Current()
		{
			if (head->next==NULL)
	                {
			    return head->val;
			}
			else
			{
			    return current->val;
			}
		};




		class Iterator
		{
			friend class List;
                        public :

			Iterator(Node* n)
			{
				it = n;
			};

		        void ShowAll()
		        {
			        while(it != NULL)
			        {
					cout << it->val << "\n";
				        it = it->next;
			        }
		        };

			void ShowFirst()
			{
				cout << it->val << "\n";
			};

		        void ShowLast()
		        {
			        while(it->next != NULL) 
				        it = it->next;
			       cout << it->val << "\n";
		        };

			~Iterator()
			{
				it = NULL;
			}

			private:
		        Node* it;
		};


                Iterator getIterator(void)
		{
			if (head == NULL)
				cout << "DS is empty!, first create a DS.";
			else
			{
				return Iterator(head);
			}
		};

		~List()
		{
			// How many iterators are connected to the list?
			head = NULL;
			current = NULL;
		}


};
		

[George P]

Your List class shouldn't care where a node is stored.

You might want to "push" your heap allocation up into your Node class. Allocate the heap memory in Node's constructor. That way when any node's destructor is called it can delete the allocated memory.

[hopeman]

Thanks for the comment. Could you give me more hint. Maybe few line of code please.

I want my List to take care of my Nodes, I want also my List to take care of its head and current pointers. That is the reason I follow this nested class structure.

Thanks.

[jonnin]

the node IS the list, so it can't just self destruct.
you are going to have to do it manually with this design.
so list destructor would do something like

while head
tmp = head;
head = head->next;
delete tmp;

[hopeman]

This logic, I was looking for. I like the idea and please let me try to make it work :)

[hopeman]

I rewrite the code. Now, the ~List() delets the entire list including all its nodes.

But, now I am wondering how the taken memory from heap, newNode, is going to be deleted?

I used the code: delete newNode; But it seems I get a runtime error.

Could someone help, please?

// Example program
#include <iostream>
#include <string>

class List
{
    public:
    
    List(int i)
    {
        Node* newNode = new Node(i);    
        if (head==NULL)
        {
              head = newNode;
              current = newNode;
        }        
        std::cout << "List created." << "\n";
        // ----> Why is not working?  --->>  delete newNode;
    };
    
    int printNode()
    {
        return current->value;
    };
    
    ~List()
    {
        Node* tmp;
        tmp = head;
        while (head)
        {
            head = head-> next;
            delete tmp;
        }
        std::cout << "List deleted." << "\n";
    };
    
    private:
    
    class Node
    {
        private:
        
        Node* next;
        int value;
        
        Node(int i)
        {
            value = i;
            next = NULL;
        };
        
        public:
        
        friend class List;
        
    };
        
    Node* head;
    Node* current;
};

using namespace std;
int main()
{
    List* mylist = new List(12);
    cout << mylist->printNode() << "\n";
    delete mylist;
}

[jonnin]

you need to step back a little and understand pointers better, maybe review your class notes.

that said:

  public:
    
    List(int i)
    {
        Node* newNode = new Node(i);    
        if (head==NULL)
        {
              head = newNode;
              current = newNode;
        }        
        std::cout << "List created." << "\n";
        // ----> Why is not working?  --->>  delete newNode;

//you DO NOT want to delete newnode here.  that also deletes HEAD.  
//newnode and head are the same memory if head is assigned, and if you 
//delete the memory of one, you delete it for both, its the SAME MEMORY
//what if head were NOT NULL here?  consider:
if (!head)
{
   head = new node(i); //get rid of newnode.
   current = head;
cout <<" List Created. \n";
}
    };

but … this is your constructor, and head is just new-variable uninitialized junk, so I would take the if statement out. Many compilers zero things in debug mode and it could stop working in release.

[hopeman]

Ah. Ok, Thanks!! Based on the previous comment I rewrite my code. Now it looks much more sexy ;)

#include <cstdlib>
#include <iostream>

using namespace std;

class List
{
	private:

		class Node
		{
			friend class List;
			private:
				Node(int Node_v)
				{
				    next = NULL;
				    val = Node_v;
				};
				~Node()
				{
				};
				
				int val;
				Node* next;
		};

		Node* head;
		Node* current;

		//friend class has no access to static member!
		//static Node* head;
		//static Node* current;
	public:

		List(int v)
		{
			head = NULL;
			current = NULL;

			head = new Node(v);
			current = head;
			cout << " Data Structure is created and initialized. \n";
		}

		void Add(int v_add)
		{
			Node* newNode = new Node(v_add);
			                         // Here I was wondering how to delete (newNode)
			                         // No need to delete it. I delete 
						 // all such newNode in ~List().

			current->next = newNode;
			current = newNode;
		};

		int Current()
		{
			if (head->next==NULL)
	                {
			    return head->val;
			}
			else
			{
			    return current->val;
			}
		};


		class Iterator
		{
			friend class List;
                        public :

			Iterator(Node* n)
			{
				it = n;
			};

		        void ShowAll()
		        {
				cout << "showing all ... " <<"\n";
			        while(it != NULL)
			        {
					cout << it->val << "\n";
				        it = it->next;
			        }
		        };

			void ShowFirst()
			{
				cout << "first: "<< it->val<< "\n";
			};

		        void ShowLast()
		        {
			        while(it->next != NULL) 
				        it = it->next;
			       cout << "last : " << it->val << "\n";
		        };

			~Iterator()
			{
				it = NULL;
			}

			private:
		        Node* it;
		};


                Iterator getIterator(void)
		{
			if (head == NULL)
				cout << "DS is empty!, first create a DS.";
			else
			{
				return Iterator(head);
			}
		};

		~List()
		{
			// How many iterators are connected to the list?
			while(head)
			{
			    Node* tmp = head;
			    cout << "deletion of--> " << head->val << "\n";
			    head=head->next;
			    delete tmp;
			}
		}


};

The only confusion I had is now cleared (hopeful). I was taking memory from heap in List::Add and I was wondering how to delete these memory places. But, now I realize that they are deleted in ~List(). Though I do not explicitly say (delete newNode) in List::Add(), but I finally delete them in ~List(). That was my confusion. Please correct me if I am wrong.

[dhayden]

A few things that you need to clarify in your design:
- Can you create an empty list? Right now that isn't possible.
- What's the difference between head and current? Right now they seem intermingled. For example, Add() adds the list to current, but not to head.
- Iterator::ShowAll() will set member it to null. But ShowLast() assumes that it isn't null.

Don't just add code to fix these things. Think about what the right answers are, add comments indicating the invariants (like if it is never null) and then make sure the code implements the design.

[hopeman]

Thanks indeed for the comments! Yes, right, I need to get myself to that level of maturity to do the pencil work first and then implement the pencil worked ideas..... Thanks once again all for helpful comments, devoted time and support :)