Overloading operator= in a singly linked list

Question

I'm currently having a problem copying the contents from one list to another. The values all appear as 0 when compiled. I thought that there might be something wrong when overloading the assignment operator as the copy constructor is using its code.

Copy Constructor

ListOfDoubles::ListOfDoubles(const ListOfDoubles&e):head(NULL) {
if (this != &e) {
    *this = e;
}
}

Overloaded operator=

const ListOfDoubles& ListOfDoubles::operator=(const ListOfDoubles 
&doubleslist)
{
DoubleListNode *cpyPtr = NULL;
DoubleListNode* orgPtr = doubleslist.head;

if (this != &doubleslist)
{
    while (head != NULL)
    {
        ListOfDoubles::~ListOfDoubles();
    }

    while (orgPtr != NULL)
    {
        if (head == NULL)
        {
            head = cpyPtr = new DoubleListNode(orgPtr->data);

        }
        else
        {
            cpyPtr->next = new DoubleListNode(orgPtr->data);
            cpyPtr = cpyPtr->next;
        }
        orgPtr = orgPtr->next;
    }
}
return *this;
}

`ListOfDoubles::~ListOfDoubles();` Bad idea. You almost never want to call the destructor yourself and this doesn't look like one of those rare times. — user4581301, Dec 05 '17 at 20:29
It causes undefined behaviour to call the destructor and then keep using the object. You are doing this all backwards; if you want to share code amongst CC and op=, then implement CC correctly , and have op= do a copy and swap — M.M, Dec 05 '17 at 20:30
This may be overkill for you, but it's reasonably easy to write and very easy to prove: [What is the copy-and-swap idiom?](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) — user4581301, Dec 05 '17 at 20:31
@user4581301 Yeah, you can check address or bind reference to objects under construction. — M.M, Dec 05 '17 at 20:36
@user - You can also write `int i = i;`. Isn't that useful either. — Bo Persson, Dec 05 '17 at 20:37
Checking for self-copying is generally a bad idea. Write your copy operations to be able to tolerate copying themselves instead. — cdhowie, Dec 05 '17 at 20:38
I knew about the `int i = i;`, but hadn't connected the dots to see that it would also apply to copy constructing one's self. — user4581301, Dec 05 '17 at 20:43
I called a clear() method rather than the destructor as they pretty much do the same thing. Is it possible to get this working without using copy-swap? — Kujinn, Dec 05 '17 at 20:43
Yes, and if you do it right it can be more efficient. But it's not as easy, and you sometimes have to explain to the code reviewers why you didn't just copy and swap. — user4581301, Dec 05 '17 at 20:45
You should ask for a code review: [codereview](https://codereview.stackexchange.com/) Couple of issues that need to be sorted. Calling destructor (without calling constructor (ends life of object without re-starting it)). Exception guarantees (If assignment fails you don't provide the strong guarantee). Premature optimization (testing for assignment to self (which is also in the wrong place)). Non standard implementation (Copy and Swap). — Martin York, Dec 05 '17 at 21:33
@user4581301 It actually is occasionally useful, as the counterpart to placement-new. (There's also placement-delete, but its function is even more obscure.) — Sneftel, Dec 05 '17 at 22:24
@Sneftel agreed, but there is almost zero chance placement new was used here and even if, that's not how to clean up afterward. — user4581301, Dec 05 '17 at 23:09

score 1 · Answer 1 · answered Dec 05 '17 at 22:00

General-purpose copy logic looks something like:

DoubleListNode * from = source.head; // copying from
DoubleListNode ** to = &head; // pointer to where we want to copy to
while (from != nullptr) // keep going until end of list. You did mark 
                        // the end of the list, didn't you?
{
    *to = new node(*from); //copy construct a new node around from and store it at to
    to = &(*to)->next; // advance to
    from = from.next; // advance from
}
*to = nullptr; // all done. Terminate list.

The real magic is going on up here at the double pointer: DoubleListNode ** to By having a pointer to a pointer, we don't care whether we're pointing at head, another node's next or what have you. It's just another node, so there are no special cases to cover.

You can do the above in both the copy constructor and assignment operator, though you are better off not repeating yourself and putting it in a function called by the copy constructor and assignment operator. Note that there are different assumptions about pre-existing data in the copy constructor (eg, list will be empty) and the assignment operator (eg, list may not be empty, so clear it and free all of the nodes before you begin) that need to be taken into account.

The primary alternative, as discussed in the comments above, is to use the Copy and Swap Idiom. For this, the above copy loop only exists in the copy constructor.

Remy Lebeau · Answer 2 · 2017-12-05T22:18:13.243

The input parameter of a copy constructor will never be the object being constructed, so checking for this != &e in the copy constructor is redundant.

Also, manually calling a destructor directly is illegal unless the memory was allocated with placement-new, which you are not using. You need to use delete to destroy your node instances.

Typically, you shouldn't implement the copy constructor in terms of operator=, you should do it the other way around. Let the copy constructor do its job of copying the source values, and then have operator= make a copy of the source list and take ownership of the copied data. This is commonly known as the "copy and swap" idiom.

Try this instead:

ListOfDoubles::ListOfDoubles()
    : head(NULL)
{
}

ListOfDoubles::ListOfDoubles(const ListOfDoubles &e)
    : head(NULL)
{
    DoubleListNode *cpyPtr = NULL;
    DoubleListNode *prevPtr = NULL;
    DoubleListNode *orgPtr = e.head;

    while (orgPtr)
    {
        cpyPtr = new DoubleListNode(orgPtr->data);

        if (!head)
            head = cpyPtr;

        if (prevPtr)
            prevPtr->next = cpyPtr;
        prevPtr = cpyPtr;

        orgPtr = orgPtr->next;
    }

    /* alternatively:

    DoubleListNode **cpyPtr = &head;
    DoubleListNode *orgPtr = e.head;

    while (orgPtr)
    {
        *cpyPtr = new DoubleListNode(orgPtr->data);
        cpyPtr = &((*cpyPtr)->next);    
        orgPtr = orgPtr->next;
    }

    *cpyPtr = NULL;
    */
}

ListOfDoubles::~ListOfDoubles()
{
    DoubleListNode *orgPtr = head;
    DoubleListNode *nextPtr;

    while (orgPtr)
    {
        nextPtr = orgPtr->next;
        delete orgPtr;
        orgPtr = nextPtr;
    }
}

ListOfDoubles& ListOfDoubles::operator=(const ListOfDoubles &doubleslist)
{
    if (this != &doubleslist)
    {
        ListOfDouble tmp(doubleslist);
        std::swap(head, tmp.head);
    }
    return *this;
}

Overloading operator= in a singly linked list

2 Answers2