CODEWITHSUNDEEP
c++pointersc++11
bj7
bj7

Reputation: 240

Test class destructor for pointer being allocated?

So I am having a problem with my code. I want to pass a value from my array of pointers to a function so the original object is not 'disturbed' (my code works perfectly fine if I pass the reference; I just am just trying to do it a different way as a learning exercise). After the implementation returns, I get an error:

"error for object 0x100105790: pointer being freed was not allocated *** set a breakpoint in malloc_error_break to debug".

I know this is because as the value goes out of scope upon the function's return, the destructor is called for the object but my destructor assumes a pointer that was allocated, thus the error. I was curious if there was a way to test if the genome was already allocated or not. If not I would do something else for the destructor? Is this even a problem worth bothering about since I already have it working by passing in the reference? The function is not actually destructive; I just have a desire to do some tricks. I don't like taking the easy way out. 

//class destructor for int genome[]
Organism::~Organism() {
    //cout << "Deleting this: " << this << endl;
    if (this->genome != NULL) {
         delete [] this->genome;
    }
}

//declare genome pointer
int *genome;
/**
 * Default constructor for class
 */
Organism::Organism() {
    this->fitness = 0;
    this->size = 0;
    this->genome = NULL;
}
//another constructor for if only the size of genome is defined
Organism::Organism(int size) {
    this->fitness = 0;
    this->size = size;
    this->genome = new int[size];
}
//another constructor for when all starting values are defined
Organism::Organism(int size, int *genome) {
    this->fitness = 0;
    this->size = size;
    this->genome = new int[size];

    for (int i = 0; i < size; i++) {
        this->genome[i] = genome[i];
    }
 }

//initialize and populate reproducible from already existing array start_pop     (this has been verified as being properly allocated and values initiated)
vector<Organism*> reproduceable (0);
    for (int i = 0; i < start_pop.size(); i++) {
        if (start_pop[i]->get_fitness() > threshold) {
            reproduceable.push_back(start_pop[i]);
        }
    }

//function definition
Organism* reproduce(Organism, Organism);

//function call in main()
offspring.push_back(reproduce(*reproduceable[i], *reproduceable[i+1]));

//function implementation
Organism* reproduce(Organism a, Organism b) {
    int genome[4];

    //randomly decide on where to split parent genomes
    int split = rand() % 5;
    for (int i = 0; i < a.get_size(); i++) {
        if (i < split) {
            genome[i] = a.get_genome()[i];
        } else {
            genome[i] = b.get_genome()[i];
        }
    }
    //now cause random mutation in 2% of the population
    if ((rand() % 100 + 1) <= 2) {
        int rand_index = rand() % 5;
        int mutation = rand() % 6 + 1;

        genome[rand_index] = mutation;
    }

    Organism *child = new Organism(4, genome); //need to add genome
    return child;
}

Edited to add default constructors and array initialization for reproducible

Upvotes: 1

Views: 310

Answers (2)

sp2danny
sp2danny

Reputation: 7687

Rule of three

If a class requires a user-defined destructor, a user-defined copy
constructor, or a user-defined copy assignment operator, it almost
certainly requires all three.

see http://en.cppreference.com/w/cpp/language/rule_of_three

Although a better approach would be rule of zero, i.e. you embrace value semantics.
For this program, it would be as simple as changing the genome member from int* to
int[4], std::array<int,4> or std::vector<int>
(and remove the destructor)

Upvotes: 1

Ishamael
Ishamael

Reputation: 12795

Your problem is in this line:

offspring.push_back(reproduce(*reproduceable[i], *reproduceable[i+1]));

Note that you pass two reproduceables by value into a function. The function's signature is

Organism* reproduce(Organism a, Organism b)

It expects two Organism's by value, which means that for both Organism's a copy constructor will be called. Since you do not define a copy constructor, a default one will be created, that just copies the content of one organism into another, including a pointer go the genome. The issue is that now both Organisms will be freed -- the one that's local to the function, and the one that was passed into it, resulting in double freeing the memory allocated for the genome. There are two ways to solve it:

  1. Make the signature to be
Organism* reproduce(const Organism &a, const Organism &b)

This way your Organism will not be copied, and thus will not be double freed. (based on your question I assume you already tried it, and are interested in the second solution :))

  1. Implement a custom copy constructor, that creates a new array, like so
Organism::Organism(const Organism& a)
{
     this->fitness = a.fitness;
     this->size = a.size;
     this->genome = new int[this->size];
     for (int i = 0; i < this->size; ++ i)
         this->genome[i] = a.genome[i];
}

This way as you pass an organism into your function, your custom copy constructor will be invoked, and the genome for the organism local to your function will be allocated separately.

Upvotes: 1

Related Questions