I have such code like,

#include <iostream>
#include <string>

using namespace std;

class Heart {
private:
    int bpm;
public:
    Heart(int bpm) : bpm(bpm) {}
    int getBPM() {
        return bpm;
    }
};

class Kidney {
private:
    double PercentFunction;
public:
    Kidney() : PercentFunction(0) {}
    Kidney(double pf) : PercentFunction(pf) {}
    double getPF() {
        return PercentFunction;
    }
};

class Person {
private:
    string fname, lname;
    int age;
    Heart h;
    Kidney* k; 

public:
    Person(string fn, string ln, int age, int bpm, double kpf1, double kpf2) : fname(fn), lname(ln), age(age), h(bpm) {
        k = new Kidney[2];
        k[0] = Kidney(kpf1);
        k[1] = Kidney(kpf2);
        cout << fname << " " << lname << ", aged " << age << ". Heart BPM : " << bpm <<
            ". Kidneys' percent function indices: " << k[0].getPF() << " and " << k[1].getPF() << '.' << endl;
    }
    ~Person() {
        cout << "A person is dying!" << endl;
        delete[] k;
    }

};

int main() {
    Person p = Person("Jack", "Bowen", 24, 60, 0.99, 0.98);
}

Then I run my code, an error(Debug Assertion Failed!) pops up. And you can also see the destructor is called twice. But if I remove the delete [] k; in the ~Person, there will be no such pop-up error.

There is dynamic allocation in the Person constructor:

k = new Kidney[2];
k[0] = Kidney(kpf1);
k[1] = Kidney(kpf2);

So I think I should delete k in the destructor. My question is why the destructor is called twice and how to solve the error?

I am using VS 2013.

Thank you!

The issue is the following. In the line

Person p = Person("Jack", "Bowen", 24, 60, 0.99, 0.98);

you are copy-initializing p, i.e. you are creating a temporary which is then copied to Person p;. At the end, the temporary Person("Jack", "Bowen", 24, 60, 0.99, 0.98); is destroyed, so your Kidney* pointer is dangling, because you didn't implement a copy constructor and the copy is shallow (i.e. the pointer itself is being copied, not the object it points to). And your destructor is called twice because it is first called when the temporary ends its life (at the end of the statement), then again when Person p goes out of scope at the end of main().

Anytime your class has a pointer, implement its copy constructor and assignment operator. Or better, use smart pointers like std::shared_ptr, or even better, standard containers that keep track of their dynamic memory like std::vector/std::list etc.

Quick and dirty fix for your code (but really, you must implement the copy constructor since you're going to have all other kinds of issues, e.g. when returning Persons from a function or when passing Persons by value):

Person p("Jack", "Bowen", 24, 60, 0.99, 0.98);

This avoids any temporary and uses direct initialization.

PS: In g++, compiling with -Weffc++ warns you about these issues,

warning: 'class Person' has pointer data members [-Weffc++] but does not override 'Person(const Person&)' [-Weffc++] or 'operator=(const Person&)' [-Weffc++]

I am not sure if such a compiler flag exists for VS though.

more