C++ newbie: destructor

Question

I'm just creating a simple list and then destroying it. And something is going wrong and I always get this annoying error message:

Expression: _BLOCK_TYPE_IS_VALID(pHead->nBlockUse)

Here's the code:

#include<iostream>
#include<Windows.h>
using namespace std;

struct node
{
    int data;
    node *next;
};

class list
{
protected:
    node *top;
public:
    list()
    {
        top=NULL;
    }

    list random()
    {
        int x=rand()%10;
        for(int i=0; i<x; i++)
        {
            node *p=new node;
            p->data=rand()%100;
            p->next=top;
            top=p;
        }
        return *this;
    }

    void show()
    {
        for(node *p=top; p; p=p->next)
        {
            cout<<p->data<<" ";
        }
        cout<<"\n";
    }

    ~list()
    {
        node *r;
        for(node *p=top; p; p=r)
        {
            r=p->next;
            delete p;
        }
    }
};

int main()
{
    srand(GetTickCount());
    list a;
    a.random().show();
    return 0;
}

You're violating the [rule of three](http://stackoverflow.com/q/4172722/636019). — ildjarn, Dec 27 '11 at 23:41

Jim Buck · Answer 1 · 2011-12-28T00:13:57.373

3

This:

list random()

should be:

list &random()

The reason is that your version returns a copy of your instance a, and that copy get destructed after show() is called.. and that destruction destroys the same memory as that a is using. If you really want to have random() return a copy, you need to implement a copy constructor that does a deep copy of the internal list that a has.

edited Dec 28 '11 at 00:13

answered Dec 27 '11 at 23:44

Jim Buck

20,482
11
57
74

score 0 · Accepted Answer · answered Dec 27 '11 at 23:44

Your problem is that you are copying your list but you don't define a copy constructor. The implicitly defined copy constructor will just copy the top pointer so you end up attempting to delete the same chain of nodes twice.

The copy occurs when you return *this; from your random() member function returning a copy of *this by value.

The shortest fix would be to make your class non-copyable by declaring a copy constructor and copy assignment operator in the private section of your class.

private:
    list(const list&);
    list& operator=(const list&);

You can then make random return void, there doesn't seem to be a good reason why it makes a copy as well.

You could then just call it like this:

    list a;
    a.random();
    a.show();

The longer fix would be to make your list copyable by making a full implementation of list(const list&) and list& operator=(const list&) that correctly duplicates all the nodes of the source list being copied.

score 0 · Answer 3 · answered Dec 27 '11 at 23:45

It's not an "annoying error message", it is telling you that your program has somehow corrupted memory. Quite important actually.

You create a copy of your list when you return *this, but you never define a copy constructor, so you end up deleting your top node twice.

C++ newbie: destructor

3 Answers3