vector related memory allocation question

Question

I am encountering the following bug.

• I have a class Foo . Instances of this class are stored in a std::vector vec of class B.
• in class Foo, I am creating an instance of class A by allocating memory using new and deleting that object in ~Foo().

the code compiles, but I get a crash at the runtime. If I disable delete my_a from desstructor of class Foo. The code runs fine (but there is going to be a memory leak).

Could someone please explain what is going wrong here and suggest a fix?

thank you!

class A{
      public:
          A(int val);
          ~A(){};
          int val_a;

};

A::A(int val){
       val_a = val;
       };

class Foo {      
      public:
             Foo();
             ~Foo();
             void createA();
             A* my_a;
};

Foo::Foo(){
    createA();
};

void Foo::createA(){
    my_a = new A(20);
};

Foo::~Foo(){
    delete my_a;

};



class B {
      public:
             vector<Foo> vec;             
             void createFoo();            
             B(){};
             ~B(){};
};


void B::createFoo(){
    vec.push_back(Foo());
};


int main(){
    B b;

    int i  =0;
    for (i = 0; i < 5; i ++){
        std::cout<<"\n creating Foo";
        b.createFoo();   
        std::cout<<"\n Foo created";
        }
    std::cout<<"\nDone with Foo creation";

    std::cout << "\nPress RETURN to continue...";
    std::cin.get();

    return 0;
}

Answer 1

You need to implement a copy constructor and an assignment operator for Foo. Whenever you find you need a destructor, you alnmost certainly need these two as well. They are used in many places, specifically for putting objects into Standard Library containers.

The copy constructor should look like this:

Foo :: Foo( const Foo & f ) : my_a( new A( * f.my_a ) ) {
}

and the assignment operator:

Foo & Foo :: operator=( const Foo & f ) {
    delete my_a;
    my_a = new A( * f.my_a );
    return * this;
}

Or better still, don't create the A instance in the Foo class dynamically:

class Foo {      
      public:
             Foo();
             ~Foo();
             void createA();
             A my_a;
};

Foo::Foo() : my_a( 20 ) {
};

Answer 2

If you don't specify a copy constructor, the compiler makes one for you. Your compiler-generated copy constructor looks like this:

Foo::Foo(const Foo& copy)
    : my_a(copy.my_a)
{}

Woops! You're copying just the pointer but not the pointed-to memory. Both your temporary Foo() in createFoo() and the one copied in to the vector point to the same memory, so the memory is deleted twice, which crashes your program on the second delete.

You should create a copy constructor that looks something like this:

Foo::Foo(const Foo& copy)
    : my_a(new A(*copy.my_a))
{}

Note this crashes if copy has a NULL my_a member, and it also invokes the copy constructor on A, which you haven't specified either. So you'll want to make some further changes. You'll also want an operator= overload too.

Answer 3

The Foo object is copied and at the destruction of every copy, delete is called on the same pointer value my_a. Implement the copy and assignment operator for Foo or use a smart pointer.

 Foo( const Foo& s) : my_a( s.my_a ? new A(*s.my_a) : 0) {
 }

 Foo& operator= (const Foo& s) {
     Foo temp(s); 
     temp.swap (*this);
     return *this;
 }

 void swap (Foo &s) { 
     std::swap (my_a, s.my_a);  
 };