1
template<typename T>
List<T>::~List()
{
    while(head->next !=NULL){
        delete head;
        head = head->next;
    }
    head = NULL;
}

I want to delete all the nodes in the linked list, but I don' t know why the code fail.

peter
  • 39
  • 1
  • 5
    `delete head; head = head->next;` is bad. You may not access `head` after `delete head;`. – Scheff's Cat Nov 29 '17 at 10:15
  • 4
    Note, that VS (in debug mode) fills deleted memory with test bit patterns to uncover [UB](https://stackoverflow.com/a/4105123/1505939). At this point, chances are high that your code crashs. (This is actually the intention.) – Scheff's Cat Nov 29 '17 at 10:17
  • A similar question (and my answer): [SO: Destructing a linked list](https://stackoverflow.com/a/45389774/7478597) – Scheff's Cat Nov 29 '17 at 10:20
  • You are also not deleting the last element of the linked list, which causes a memory leak. – Pedro Nov 29 '17 at 12:33

4 Answers4

2

Your code fails probably because it invokes undefined behaviour.

delete head;
head = head->next;

You cannot read the memory located where head points after having deleted head.

You should make a copy of head->next to reuse it:

const auto next = head->next;
delete head;
head = next;
YSC
  • 38,212
  • 9
  • 96
  • 149
  • 1
    @peter auto is a C++11 addition to let the compiler infer (guess) the type of the defined variable. If you wish not use it, replace `auto` with the type of `head->next` (I cannot know it from your question). – YSC Nov 29 '17 at 10:28
  • while(head->next!=NULL){ Node*next = head->next; delete head; head = next; } head = NULL; is it correct now? – peter Nov 29 '17 at 10:41
  • @peter yes. `head` is a `Node *`, and so is `Node::next`? – Caleth Nov 29 '17 at 10:43
  • That fixes the invalid memory access, but leaves the issue with the leaking memory of the last element. – Pedro Nov 29 '17 at 12:32
  • @PedramAzad the memory leak is indeed good to mention, and I forgot about it. You might write an answer, I'll be the first one to upvote it. – YSC Nov 29 '17 at 12:35
1

I would go with the following solution. No invalid memory access, no memory leak, and it automatically assigns NULL to head before leaving the loop:

template<typename T>
List<T>::~List()
{
    while (head)
    {
        Node<T> *next = head->next;
        delete head;
        head = next;
    }
}

Please note that I made a guess with the node type and you have to replace it by whatever is present in your code.

Pedro
  • 842
  • 6
  • 16
0

This can help you in deleting every node of linked list.

   List<T> *current = head;
   while (current != NULL) 
   {
       next = current->next;
       delete current;
       current = next;
   }
Bhawan
  • 2,441
  • 3
  • 22
  • 47
0

It depends what type the head variable is. If it is the class, you can even use "recursive" deleting in "head" (I mean node) destructor. Something similar to:

Node::~Node(){
    if(this->next != nullptr)
        delete this->next;
    delete this->content;
}

If the node is internal struct, you need temporary pointer to next node as mentioned above:

template<typename T>
List<T>::~List()
{
    struct nodeType *temp;
    while(head != nullptr){
        temp = head->next;
        delete head;
        head = temp;
    }
}

In this case, you don't need to explicitly set head to null, since it is done by the logic of while loop.

In your loop (in question) you don't remove *head node, I don't know if it is expected behavior.

Ad. comment above (I can't put it directly): while(head->next!=NULL){ Node*next = head->next; delete head; head = next; } head = NULL; is wrong. It doesn't delete last node on the list (especially if the list has only one object, it won't be deleted).

Karol T.
  • 543
  • 2
  • 13