How to destroy a vector of pointers in c++?

Question

I have the following code in one of my methods:

vector<Base*> units;
Base *a = new A();
Base *b = new B();
units.push_back(a);
units.push_back(b);

Should I destroy the a and b pointers before I exit the method? Or should I somehow just destroy the units vector of pointers?

Edit 1:

This is another interesting case:

vector<Base*> units;
A a;
B b;        
units.push_back(&a);
units.push_back(&b);

What about this case? Now I don't have to use delete nor smart pointers.

Thanks

Answer 1

Warning against your second example.

This simple extension leads to undefined behavior:

class A {
 public:
  int m;
  A(int _m): m(_m) {}
};

int main(){
  std::vector<A*> units;
  for (int i = 0; i < 3; ++i) {
    A a(i);
    units.push_back(&a);
  }
  for (auto i : units) std::cout << i->m << " "; // output: 2 2 2 !!!!
  return 0;
}

In each loop, the pointer to each a is saved in units, but the objects that they point to go out of scope. In the case of my compiler, the memory address of each a was re-used each time, resulting in units holding three identical memory addresses -- all pointing to the final a object.

Answer 2

In the first example, you will eventually have to delete a and b, but not necessarily when units goes out of scope. Usually you will do that just before units goes out of scope, but that is not the only possible case. It depends on what is intended.
You might (later in the same function) alias a or b, or both, because you want them to outlive units or the function scope. You might put them into two unit objects at the same time. Or, many other possible things.

What's important is that destroying the vector (automatic at scope end in this case) destroys the elements held by the vector, nothing more. The elements are pointers, and destroying a pointer does nothing. If you also want to destroy what the pointer points to (as to not leak memory), you must do that manually (for_each with a lambda would do).
If you don't want to do this work explicitly, a smart pointer can automatize that for you.

The second example (under Edit1) does not require you to delete anything (in fact that's not even possible, you would likely see a crash attempting to do that) but the approach is possibly harmful.

That code will work perfectly well as long as you never reference anything in units any more after a and b left scope. Woe if you do.

Technically, such a thing might even happen invisibly, since units is destroyed after a, but luckily, ~vector does not dereference pointer elements. It merely destroys them, which for a pointer doesn't do anything (trivial destructor).
But imagine someone was so "smart" as to extend the vector class, or maybe you apply this pattern some day in the future (because it "works fine") to another object which does just that. Bang, you're dead. And you don't even know where it came from.

What I really don't like about the code, even though it is strictly "legal" is the fact that it may lead to a condition which will crash or exhibit broken, unreproducable behaviour. However, it does not crash immediately. Code that is "broken" should crash immediately, so you see that something is wrong, and you are forced to fix it. Unluckily that's not the case here.

This will appear to work, possibly for years, until one day it doesn't. Eventually you'll have forgotten that a and b live on the current stack frame and reference the non-existing objects in the vector from some other location. Maybe you dynamically allocate the vector in a future revision of your code, since you pass it to another function. And maybe it will continue to appear working.
And then, you'll spend hours of your time (and likely the time of others) trying to find why a section of code that cannot possibly fail produces wrong results or crashes.

Answer 3

A rather old-fashioned solution, that works with all compilers:

for ( vector<Base*>::iterator i = units.begin(); i != units.end(); ++i )
    delete *i;

In C++11 this becomes as simple as:

for ( auto p : units )
    delete p;

Your second example doesn't require pointer deallocation; actually it would be a bad error to do it. However it does require care in ensuring that a and b remain valid at least as long as units does. For this reason I would advise against that approach.

Answer 4

If you exit the method, units will be destroyed automatically. But not a and b. Those you need to destroy explicitly.

Alternatively, you could use std::shared_ptr to do it for you, if you have C++11.

std::vector<std::shared_ptr<Base>> units;

And you just use the vector almost as you did before, but without worrying about memory leaks when the function exists. I say almost, because you'll need to use std::make_shared to assign into the vector.

Answer 5

The best way to store pointers in a vector will be to use smart_ptr instead of raw pointers. As soon as the vector DTOR is called and control exits the DTOR all smart_ptrs will be refernced counted. And you should never bothered about the memory leak with smart_ptrs.

Answer 6

Yes and no. You don't need to delete them inside the function, but for other reasons than you might think.

You are essentially giving ownership of the objects to the vector, but the vector is not aware of that and therfore wont call delete on the pointers automatically. So if you store owning raw pointers in a vector, you have to manually call delete on them some time. But:

1. If you give the vector out of your function, you should not destroy the objects inside the function, or the vector full of pointers to freed memory would be pretty useless, so no. But in that case, you should make sure the objects are destroyed after the vector has been used outside the function.
2. If you don't give the vector out of the function, you should destroy the objects inside the function, but there would be no need to allocate them on the free store, so don't use pointers and new. You just push/emplace the objects themselves into the vector, it takes care of the destruction then, and therfore you don't need delete.

And besides that: Don't use plain new. Use smart pointers. Regardless what you do with them, the smart pointers will take care of a proper destruction of the objects contained. No need to use new, no need to use delete. Ever. (Except when you are writing your own low level data structures, e.g. smart pointers). So if you want to have a vector full of owning pointers, these should be smart pointers. That way you won't have to worry about wether, when and how to destroy the objects and free the memory.

Answer 7

You have to delete them unless you will have memory leak , in the following code if I comment the two delete lines the destructors never called, also you have to declare the destuctor of the Base class as virtual. As others mentioned is better to use smart pointers.

#include <iostream>
#include <vector>

class Base
{
public:
  virtual ~Base(){std::cout << "Base destructor" << std::endl;};
};

class Derived : public Base
{
  ~Derived(){std::cout << "Derived destructor" << std::endl;};
};

int main()
{
  std::vector<Base*> v;
  Base *p=new Base();
  Base *p2=new Derived();
  v.push_back(p);
  v.push_back(p2);

  delete v.at(0);
  delete v.at(1);
};

Output:

Base destructor
Derived destructor
Base destructor

Output with non-virtual base destructor (memory leak):

Base destructor
Base destructor

Answer 8

I would suggest that you use SmartPointers in the vector. Using smart pointers is a better practice than using raw pointers. You should use the std::unique_ptr, std::shared_ptr or std::weak_ptr smart pointers or the boost equivalents if you don't have C++11. Here is the boost library documentation for these smart pointers.

In the context of this question, yes you have to delete the pointers that are added to the vector. Else it would cause a memory leak.

Answer 9

You should not delete if this two situation match.

1. Created vector return to out side of the function.
2. Vector created outside of the function and and suppose to access from other functions.

In other situations you should delete memory pointed by pointers in vector. otherwise after you delete the pointers, no way to refer this memory locations and it calls memory leak.

vector<Base*>::iterator it;

for ( it = units.begin(); it != units.end(); ){
      delete * it;         
}

Answer 10

Yes you should destroy those pointers (assuming you aren't returning the vector elsewhere).

You could easily do it with a std::for_each as follows:

std::for_each( units.begin(), units.end(), []( Base* p ) { delete p; } );

Answer 11

You need to iterate over the vector and delete each pointer it contains. Deleting the vector will result in memory leaks, as the objects pointed to by its elements are not deleted.

TL;DR: The objects remain, the pointers are lost == memory leak.