Assignment operator creates pointer and doesn't make deleting possible

Question

As a part of a school assignment, we have to crate an abstract class and use a register class to contain them. The abstract class has two under classes. Like Animal > Dog/Cat

In this task we have to make an assignment operator but after using the one I made, the program gets an issue.

I make two registers

r1; r2;

then use the operator

r2 = r1;

and when the program exits, it goes to the destruktor of r1, removes it, gets to r2 and gets an "Access violation reading location"

I'm guessing this is because the operator creates a pointer from r2 to r1, so when r1 has been deleted.

Operator:

Register& Register::operator=(Registerconst &other)
{
    for (int i = 0; i < this->count; i++)
    {
        delete this->animals[i];
    }
    delete[] this->animals;
    this->name = other.name;
    this->size = other.size;
    this->count = other.count;
    this->animals= new Animal*[this->size];
    for (int i = 0; i < this->count; i++)
    {
        animals[i] = other.animals[i];
    }
    for (int i = count; i < this->size; i++)
    {
        this->animals[i] = nullptr;
    }
    return *this;
}

The destructors aren't virtual. not sure if that's causing it

Due to request here's the place where it's being used

AnimalRegister r1("Name 1");
AnimalRegister r2("Name 2");
// some stuff being added to r1
r2 = r1;
return 0;

the constructor:

AnimalRegister::AnimalRegister(string name)
{
    this->name = name;
    this->size = 10;
    this->count = 0;
    this->animals = new Animal*[this->size];
    for (int i = 0; i < this->size; i++)
        animals[i] = nullptr;
}

The destructor:

AnimalRegister::~AnimalRegister()
{
    for (int i = 0; i < this->count; i++)
        delete animals[i];
    delete[] animals;
}

Is this just fancy OOP terminology that I don't know? What is a "register class"? — Cody Gray - on strike, Feb 14 '16 at 17:09
What you want is called [shared pointer](http://en.cppreference.com/w/cpp/memory/shared_ptr). You can read _implementation detail_ section to find ideas, if you want to make your own. — Lol4t0, Feb 14 '16 at 17:17
@CodyGray probably just translation issue if assignment originally is not in English — Lol4t0, Feb 14 '16 at 17:18
sorry, called it a register class due to the name, It has these variables and is used as a register of all animals: string name; int size; int count; Animal** animals; — Frank Hansen, Feb 14 '16 at 17:20
Post destructors of register class and its derivatives.Anyway, destructor should be virtual for proper destruction of derived to work. — Zereges, Feb 14 '16 at 17:23
So should I make the AnimalRegister's destructor virtual? or just the Animal, dog and cat destructors? — Frank Hansen, Feb 14 '16 at 17:24
Consider using [copy and swap](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) to make your assignment operator. That doesn't avoid the problem you are having, but it means the problem will only occur in one place (the copy constructor). As you have it now, the same problem occurs in both the copy constructor and the assignment operator. — M.M, Feb 14 '16 at 21:02
For someone to answer this question, you need to describe what you want the assignment operator to do. Say you have an animal register, and you make a second animal register (using copy constructor or assignment operator). Should both registers point to the same animal instance? (i.e. editing an animal in the first register causes the corresponding animal in the second register to be updated)? Or should it create two completely separate entities of register+animal ? — M.M, Feb 14 '16 at 21:05

myrkle · Accepted Answer · 2016-02-14T19:59:28.840

1

The reason you are getting an access violation is that you are trying to delete the Animal pointers twice. Your Register assignment operator copies the Animal pointers from source object to the destination object. The destructor of the Register class deletes the Animal pointers it holds -- since these are the same Animal pointers in r1 and r2 you get an access violation when you try and delete them the second time.

You need to decide who owns the Animal pointers. Your question doesn't give me enough information to determine that.

If something outside the Register classes owns the Animals, the Register class should not be deleting them.

If the register class does own them, you need to make deep copies of the animals through some sort of virtual clone() method on Animal.

If the ownership is shared, you should be using std::shared_ptr to hold the Animals.

edited Feb 14 '16 at 19:59

answered Feb 14 '16 at 17:41

myrkle

26
2

I've added the constructor and destructor to my origional post. Shouldn't the assignment operator make r2 become a copy of r1 instead? Or it it supposed to make it into a pointer? Maybe if I created r2 before r1, that was r2 would be deleted first instead? – Frank Hansen Feb 14 '16 at 18:16
@Frank Hansen It depends on what you mean by "make a copy of". If each register should own its own copy of the animals it holds, then when you copy a register you need to create a new copy of each animal that the register will hold (this is the "deep copy" I mention in my answer). If the register is just a set of animals from some master list of animals stored somewhere else, the master list of animals should delete the animals or the animals should be stored in shared_ptrs. – myrkle Feb 14 '16 at 18:36
Upon reading on the subject more, I found out I needed a clone/copy function that returns a new instance of the object Cat/Dog. – Frank Hansen Feb 16 '16 at 08:54

Assignment operator creates pointer and doesn't make deleting possible

1 Answers1