CODEWITHSUNDEEP
c++visual-c++destructor
Adel Nizamuddin
Adel Nizamuddin

Reputation: 821

C++ newbie: destructor

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;
}

Upvotes: 1

Views: 143

Answers (3)

Jim Buck
Jim Buck

Reputation: 20726

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.

Upvotes: 3

Ed Swangren
Ed Swangren

Reputation: 124642

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.

Upvotes: 0

CB Bailey
CB Bailey

Reputation: 791869

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.

Upvotes: 0

Related Questions