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();
}

Yes, this is the correct way to do it, and practically the only way to do it before c++11. In c++11 you can just use vector> and it will free automatically — Alex Telishev, Feb 03 '14 at 18:07
_'In A's destructor, is the following correct to free up everything?'_ Yes, it's correct. Why aren't you check up this yourself using a good memory leak detection tool like [valgrind](http://valgrind.org/)? — πάντα ῥεῖ, Feb 03 '14 at 18:07
It looks OK. Is there a reason that you think it's not correct? — Component 10, Feb 03 '14 at 18:07

score 5 · Accepted Answer · edited May 23 '17 at 10:28

5

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.

edited May 23 '17 at 10:28

Community

1
1

answered Feb 03 '14 at 18:10

Mike Seymour

249,747
28
448
644

@Mike Seymour Should I check for null pointer before `delete m_member_A[i]`, just to be safe? – ontherocks Feb 04 '14 at 15:23
@ontherocks: No, it's safe to give `delete` a null pointer; it will do nothing with it. – Mike Seymour Feb 04 '14 at 15:24
@Mike Seymour. Thanks. If `m_member_A` was a `std::map`, and that was the only member of A, would I need to implement A's destructor like `A::~A(){ m_member_A.clear(); }` or this is redundant too? If it is redundant then I guess the default destructor would suffice. – ontherocks Feb 04 '14 at 15:58
@ontherocks: Indeed, that's redundant. You should never need to do anything when destroying a properly designed class; that's the whole point of having a destructor. – Mike Seymour Feb 04 '14 at 16:01

score 4 · Answer 2 · answered Feb 03 '14 at 18:07

4

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.

answered Feb 03 '14 at 18:07

Lightness Races in Orbit

378,754
76
643
1,055

score 3 · Answer 3 · 2014-02-03T18:48:36.027

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)

score 2 · Answer 4 · answered Feb 03 '14 at 18:09

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.

Delete vector class member

4 Answers4

Linked