What's wrong with my LinkedList assignment operator?

Question

This throws a segmentation fault.

LList<T>& LList<T>::operator=(LList s){

    LList<T> final_list;
    final_list.head_ = s.head_;
    Node<T> *f_curr_node = final_list.head_;
    Node<T> *s_curr_node = s.head_;

    for (int x=0; x<s.length();x++){
        f_curr_node->next_ = s_curr_node->next_; 
    }

    return final_list;
}

I do not understand why this is erroring out / how to fix it.

Provide a [http://stackoverflow.com/help/mcve](http://stackoverflow.com/help/mcve) at least please. — πάντα ῥεῖ, Dec 02 '14 at 20:59
Also see http://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope — πάντα ῥεῖ, Dec 02 '14 at 21:00
Aside from the obvious UB explained in the answer below, what are you trying to achieve with that assignment operator definition? Any reasonable assignment operator would leave the operand in the same state as the source object, so then why are you creating a new object instance within the body instead of assigning the data members of the current instance? And are you sure you want to make a copy of the source object you're assigning from? — Praetorian, Dec 02 '14 at 21:07

score 4 · Accepted Answer · edited Dec 02 '14 at 21:07

4

You are returning a reference to a local variable. As soon as the operator function ends, that variable goes out of scope and is destructed. If anyone tries to use what is returned, he will be accessing unallocated memory.

There are lots of other problems with this code after this fix, but that is most likely the one that causes you to crash.

edited Dec 02 '14 at 21:07

Captain Giraffe

14,407
6
39
67

answered Dec 02 '14 at 21:00

Daniel

6,595
9
38
70

PaulMcKenzie · Answer 2 · 2014-12-03T01:42:57.697

As others have pointed out, you are returning a reference to a local variable, which is undefined behavior.

Since you're passing the object in your assignment operator by value, this indicates that you have written a working copy constructor and destructor for your linked list class. If the copy constructor and destructor are not working or not completed, then right away, your code is broken even before the return statement.

But if we are to assume that your copy constructor and destructor are implemented, working, and have no bugs, here is an easier way to write your assignment operator:

LList<T>& LList<T>::operator=(LList s){
    std::swap(s.head_, head_);
    std::swap(s.length, length);
    // swap out any other members
    return *this;
}

I don't know your other member variables, so you need to swap those also. Basically all we do is take the list you passed by value (again, this could only work if your copy constructor is working), and swapped out the internals of s with the current object. Then when s dies (which is why you must have a working destructor for LList), it takes along with the old data, while the new data sits in this.

Then we return *this, which is what you should have returned in your initial attempt, and not a reference to a local variable.

Look up the copy/swap idiom for writing assignment operators.

What's wrong with my LinkedList assignment operator?

2 Answers2