Destructor deletes copy in function returning dynamic structure

Question

Ok everyone, noob question.

So I have a template class implementing a singly linked list. A function in a class in my program returns one of these lists.

psList<int> psObj::getList() const {
 return List;
}

So what is happening is on the call to return List the copy constructor kicks in which does its job nicely and creates a copy of the list. However then the function finishes and goes out of scope and calls the Destructor! All of a sudden the returned linked list gets deleted, as this is what my destructor does, deletes a list and deletes it well.

I understand I could just make the return type a pointer to the head of the copied list and all would be well and good, but the trouble is I would still not be able to create a function returning a copy of a dynamic structure, even if I wanted to, and I do want to.

I was asked for more code.

Here is the copy constructor, which obviously does a deep copy

template<class psClass>
psList<psClass>::psList(const psList &original) {
    head = NULL;

    if(original.head != NULL) {
        psNode<psClass>* iterator = original.head;
        while(iterator != NULL) {
            pushback(iterator->data);
            iterator = iterator->next;
        }
    }
}

Here is the destructor

template<class psClass>
psList<psClass>::~psList() {
    erase();
}

Here is the erase function the destructor calls.

template<class psClass>
void psList<psClass>::erase() {
    psNode<psClass>* iterator = head;
    psNode<psClass>* buff;

    while(iterator != NULL) {
        buff = iterator->next;
        delete iterator;
        iterator = buff;
    }
}

So yes I am doing deep copies and deep destructs. The problem is not the depth. The problem is thus. In the original function a deep copy is made and returned. The function goes out of scope and the deep destructor is called on the copy. No more copy.

To better explain here is what it looks like in the debugger

The original list before the getlist function call.

head 0x616080
data 2
next 0x616060
data 12
next 0x0

Here is the List of "return List" once inside the getList function

head 0x616080
data 2
next 0x616060
data 12
next 0x0

Same thing.

Here are the lists "original" and "this" at the end of the copy constructor.

"this"

head 0x63c900
data 2
next 0x63a940
data 12
next 0x0

"original"

head 0x616080
data 2
next 0x616060
data 12
next 0x0

Everything looks great doesn't it.

Now we are back in the getList function and about to step into the final bracket.

psList<int> psObj::getList() const {
 return List;
} // This bracket

The list List back in this function is what you would expect it to be

head 0x616080
data 2
next 0x616060
data 12
next 0x0

And now that we step into the final bracket the destructor is called where there is

/* 
 * No idea what the in chrg thing is or why the debugger is telling me it was
 * optimized out but I mentioned it here cause maybe it has something to do with my
 * problem
 */
this 0x7ffffffe650
__in_chrg value optimized out

// Look familiar? well it should cause it is the head of the list I returned.
head 0x63c900 
data 2
next 0x63a940
data 12
next 0x0

Then bam! The list I just copied and returned gets deleted by the destructor cause it goes out of scope.

To reiterate my original question after that detour. How do I get a dynamic structure to be returned by a function using a deep copy, without having the destructor destroy the said copy.

More code on request

// Simple single link node with default constructor initializing the link to NULL.
template <class psClass>
struct psNode {
    psClass data;
    psNode<psClass>* next;

    psNode() {
        next = NULL;
    }
};

and the push back function

template<class psClass>
void psList<psClass>::pushback(psClass object) {
    psNode<psClass>* ptr = new psNode<psClass>;
    ptr->data = object;

    if(head == NULL)
        head = ptr;
    else {
            //Have to find the tail now
        psNode<psClass>* tail;
        psNode<psClass>* iterator = head;
        while(iterator != NULL) {
            tail = iterator;
            iterator = iterator->next;
        }
        tail->next = ptr;
    }
}

And yes I know keeping track of tail would be easier.

Here is the psList class definition:

template <class psClass>
class psList {
public:
    psList();
    ~psList();
    psList(const psList &original);
    psList(psNode<psClass>* _head);

    void erase();
    void pushfront(psClass object);
    void pushback(psClass object);
    bool isEmpty() const;
    psNode<psClass>* front() const;

private:
    psNode<psClass>* head;
};

No overloaded assignment operator yet. I plan to add it in after I jump over this hurdle.

Answer 1

It would appear that psList's copy constructor makes a shallow copy instead of a deep one. In general, if you manage resources in a class, then you need non-trivial copy constructor, assignment operator and destructor (the "big three"). Please show us the code of psList.

Answer 2

What you are currently doing is effectively as follows:

psList<int> psObj::getList() const { return psList<int>(List); }

This creates a copy of the member List and copies it across to the frame where getList was called. The way that it is copied across depends upon how you call it. If you construct a new psList object from this data, as in

psList<int> newList = obj.getList(); // same as psList<int> newList(obj.getList());

the copy constructor is used. Often the two copies are reduced to a single copy by RVO.

Alternatively, if you are copying to an existing object, as in

psList<int> newList;
newList = obj.getList();

the original object's state is replaced with the data from the returned result, via the assignment operator. If you don't declare one of your own, the compiler will define a public assingment operator for you. But this is simply going to be a copy of each member of your object. That is:

psList & psList::operator=(const psList& src) {
  head = src.head;
}

so in the calling code what happens is as follows:

psList<int> newList; // psList default constructor called
newList = obj.getList(); // 1) obj.List copied via copy constructor within getList
                         // 2) copy of obj.List copy-assigned to newList (simple copy of head pointer)
                         // 3) copy of obj.List destructed
// newList now has head pointing to destroyed data

which in your case is not what you want, and what you should have done was made sure the copy assignment genuinely peformed the intended deep copy (see copy-and-swap for a method to do so via the copy constructor you've already implemented).

Hence the rule of three: if you need to define your own implementation of any one of the destructor, copy constructor and copy assignment, then you need to define them all (or at least declare the copy assignment and copy ctor private to render your class uncopyable).

As an aside, why not return a reference:

const psList<int> & psObj::getList() const { return List; }

and make the calling function decide whether to make a copy?

psList<int> localList(localPsObj.getList());