return *this causing major frustration

Question

I've been using C for about a year and finally decided to learn C++. I'm trying to implement my own linked list class like this:

#include <cstdio>
#include <cstdlib>

struct linked_list_node
{
    int value;
    struct linked_list_node* next;
};

class LinkedList
{
    linked_list_node* head;
public:
    LinkedList add(int value)
    {
        printf("add called\n");
        if (head == nullptr)
        {
            head = new linked_list_node;
            head->value = value;
            head->next = nullptr;
        }
        else
        {
            linked_list_node* curr = head;
            while (curr->next != nullptr) curr = curr->next;
            curr->next = new linked_list_node;
            curr->next->value = value;
            curr->next->next = nullptr;
        }
        return *this;
    }
    int get(size_t index)
    {
        linked_list_node* curr = head;
        for (int i = 0; i < index; i++) curr = curr->next;
        return curr->value;
    }
    LinkedList()
    {
        head = nullptr;
    }
    ~LinkedList()
    {
        linked_list_node* curr = head, * temp;
        while (curr != nullptr)
        {
            temp = curr;
            curr = curr->next;
            delete temp;
        }
    }
};

int main()
{
    LinkedList l;
    l.add(1);
    l.add(2);
    printf("%d\t%d\n", l.get(0), l.get(1));
    return 0;
}

This causes an exception to occur. I've found the mistake, which was the method add returning *this. This caused the destructor to be called after l.add(1), which deleted the allocation for head and screwed up the whole program. After defining the add method as:

void add(int value) // void method
{
    printf("add called\n");
    if (head == nullptr)
    {
        head = new linked_list_node;
        head->value = value;
        head->next = nullptr;
    }
    else
    {
        linked_list_node* curr = head;
        while (curr->next != nullptr) curr = curr->next;
        curr->next = new linked_list_node;
        curr->next->value = value;
        curr->next->next = nullptr;
    }
    // doesn't return *this
}

the code ran flawlessly. Can someone please explain why returning *this causes the destructor to get called. Thanks in advance :)

Not the `*this` alone is the problem but in combination with return type `LinkedList`. The latter is return by value and this causes a copy of your instance. (Maybe, your copy constructor is flawed as well.) With `LinkedList&`, this issue would probably vanish again but I would keep the copy constructor in mind... — Scheff's Cat, Jun 13 '21 at 13:52
The `add` method's returned `LinkedList` is *copied*. That copy is not not used by the caller, and that copy gets destructed. Also, be aware that `std::list` is already part of C++, and so there is no need to write your own (and with bugs). — Eljay, Jun 13 '21 at 13:56
At the 2nd glance, I see that you don't have a copy constructor. In this case, the compiler creates a default copy constructor for you, and this makes two owners of the same `head`. Here we are: The temporary (returned by `add()`) is destructed soon and deletes your list. The other instance has now a dangling pointer in `head` and the Undefined Behavior starts to evolve. Important reading: [Rule of 0/3/5](https://en.cppreference.com/w/cpp/language/rule_of_three). — Scheff's Cat, Jun 13 '21 at 13:56
Related: [https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — drescherjm, Jun 13 '21 at 14:02

Mark Ransom · Accepted Answer · 2021-06-13T15:47:39.140

5

Your add function was returning a copy of LinkedList. Since you didn't provide a copy constructor, you were using the compiler-generated one which simply copies the members element-by-element. When you copy pointers, there's a huge danger of doing a double free or trying to use a pointer after it's been freed.

There's a guideline called the "rule of three" (see What is The Rule of Three?) that says if you have a constructor or destructor, you probably need to have a copy constructor too. It's meant to keep you out of this kind of trouble.

edited Jun 13 '21 at 15:47

answered Jun 13 '21 at 13:56

Mark Ransom

299,747
42
398
622

Mentioning rule of 0/3/5 would improve this answer. – Yakk - Adam Nevraumont Jun 13 '21 at 14:50
@Yakk-AdamNevraumont always meant to include it in the answer, but wanted a good link to explain it first. Was a little rushed so it had to wait, but it's there now. If you follow the link you'll also see updated information about the rule of 0/3/5. – Mark Ransom Jun 13 '21 at 15:48

score 3 · Answer 2 · answered Jun 13 '21 at 13:56

Can someone please explain why returning *this causes the destructor to get called.

You were returning LinkedList by value. This means a copy was created. The pointer member was copied. Then the destructor was called on the original and the copy. The first call frees the list. The second call tries to do it again and breaks.

For a linked list like this you normally want to implement the copy constructor, the move constructor, the copy assignment operator and the move assignment operator.

return *this causing major frustration

2 Answers2