Delete vector class member

Question

I have a class A with a member which is a vector of object pointers of another class B

class A
{
    std::vector<B*> m_member_A

m_member_A is populated by creating objects of B by using new operator

B* b1 = new B;
m_member_A.push_back(b1);

In A's destructor, is the following correct to free up everything?

A::~A()
{
    for(int i = 0; i < m_member_A.size(); ++i)
    {
        delete m_member_A[i];
    }

    m_member_A.clear();
}

Answer 1

It's correct, as long as you also have a correct copy constructor and copy-assignment operator per the Rule of Three. Note that the clear() is redundant, since the vector's destructor will release its memory.

By why are you messing around with pointers and new? Why not follow the Rule of Zero, and use vector<B>, or vector<unique_ptr<B>> if you need pointers for polymorphism? Then you shouldn't need to worry about a destructor, copy constructor or copy-assignment operator at all; and you'll get move semantics as a bonus.

Answer 2

Yes, it's correct… yet it is not sufficient.

You will also need to deep-copy the container whenever your A is copied.

If you can use smart pointers inside the vector, then so much the better. Just be clear in your mind and in your code about who owns what.

Answer 3

Almost

If you construct (allocate) elements of B in the constructor of A and some B is throwing an exception you have a memory leak (unless it is caught in the constructor of A where deletion of B's is done)

And the usual rule of three.

Some illustration might be (please adjust MAKE_FAILURE to 0 or 1):

#include <iostream>
#include <stdexcept>
#include <vector>

#define MAKE_FAILURE 0

static unsigned count;
const unsigned ExceptionThreshold = 3;
struct B {
    B() { if(ExceptionThreshold <= count++) throw std::runtime_error("Sorry"); }
    ~B() { std::cout << "Destruct\n"; }
};

struct A
{
    std::vector<B*> v;
    A()
    {
        for(unsigned i = 0; i < ExceptionThreshold + MAKE_FAILURE; ++i)
            v.push_back(new B);
    }

    ~A()
    {
        for(unsigned i = 0; i < v.size(); ++i) {
            delete v[i];
        }
    }
};

int main()
{
    A a;
    return 0;
}

Also you get into the terrain of shallow and deep copies (as mentioned by @ Lightness Races in Orb)

Answer 4

This is correct way to free memory of your dynamically allocated objects in m_member_A vector. You actually dont need to call:

m_member_A.clear();

std::vector will free its memory on its own.

If you can use C++11, then I would suggest changing to shared_ptr:

  std::vector<std::shared_ptr<B>> m_member_A

now you dont need to free memory on your own.