Calling destructor after shallow-copy gives "Abort trap: 6" error

Question

So I have a self-implemented linkedlist class. For the copy constructor which makes a "deep copy" or basically a copy of the linkedlist. But for the assignment operator, I have basically done this

// assignment operator
linked_list& linked_list::operator=(const linked_list& L){

    // check for self assignment 
    if(this != &L){

        this->head=L.head;
        this->sz=L.sz;
    }

   return *this;    
}

This makes a "shallow copy", so when I make a new linkedlist b and point it to a, changes in b will be reflected in a.

But now the issue is, when I call a destructor which dynamically allocates the nodes, since both of them point reference the same object, and one of the destructors gets called before the other, I get the following error message.

edit.out(3537,0x106bcbdc0) malloc: *** error for object 0x7fa430c018f0: pointer being freed was not allocated

edit.out(3537,0x106bcbdc0) malloc: *** set a breakpoint in malloc_error_break to debug Abort trap: 6

I used lldb to probe any solutions, but it is beyond my control to estimate which objects destructor will be called first, and how I can prevent it from being called again.

You should read up on the [rule of 3/5/0](https://en.cppreference.com/w/cpp/language/rule_of_three). — G.M., Apr 20 '20 at 21:32
An assignment operator that does a shallow copy doesn't make sense. If you do `a = b` you would expect `a` and `b` to contain separate copies instead of both linking to one copy. If you wanted two names for one copy you would use a reference, and if you want to transfer ownership of one list without making a copy you would do a move. — eesiraed, Apr 20 '20 at 21:33
Does you copy constructor work correctly? If yes, does your destructor work correctly? If yes, then the assignment operator is just a copy and 2 swap statements, practically similar to what you tried, but using `std::swap`. — PaulMcKenzie, Apr 20 '20 at 21:37
Also, as the comment suggested, an assignment operator doing a shallow copy is counter-intuitive and honestly, is a bug if the copy constructor does non-shallow copying. A programmer who does this `linked_list one; linked_list two(one);` expects similar copy semantics to `linked_list one; linked_list two; two = one;` — PaulMcKenzie, Apr 20 '20 at 21:48

score 2 · Accepted Answer · edited Apr 20 '20 at 23:53

If your copy constructor and destructor are working properly (meaning no bugs), then the assignment operator can be easily written using the copy / swap idiom:

#include <algorithm>
//...
linked_list& linked_list::operator=(const linked_list& L)
{
    if (this != &L)
    {
        linked_list temp(L);
        std::swap(head, temp.head);
        std::swap(sz, temp.sz);
    }
    return *this;
}

I am assuming that head and sz are the only members. If you have other members, you need to swap those also.

In a nutshell, the way this works is that you make a copy of the passed-in linked_list. Then you simply swap out the current object's internals with the temporary copy's internals. Then the temporary copy destructs, along with the old internals.

Calling destructor after shallow-copy gives "Abort trap: 6" error

1 Answers1