Memory leaks in a pointer vector

Question

I'm making a program that uses plenty of vectors containing pointers and I'm concerned that the way i delete the pointers might cause memory leaks down the line.

Here's for example how I add a bullet to the vector:

Bullet* aNewBullet;
aNewBullet = new Bullet(x,y,dirX,dirY,size,duration);
aNewBullet->assignTexture(btext);
bulletVector.push_back(aNewBullet);

And here's how i delete bullets from the vector (for example, if it exceeds its range)

int i=0;
while (i<bulletVector.size()){
    if (bulletVector[i]->getDuration() ==0)
        bulletVector.erase(bulletVector.begin() + i);
    else i++;
}

Am I deleting the bullet by telling the vector to erase? Or should i do

int i=0;
while (i<bulletVector.size()){
    if (bulletVector[i]->getDuration() ==0)
        delete bulletVector[i];
        bulletVector.erase(bulletVector.begin() + i);
    else i++;
}

instead? Does this mess up the vector size by deleting the pointer first, and then deleting the record from the vector again?

Also does deleting the pointer call the bullet destructor (~Bullet())?

Answer 1

In the first code example you are not deleting any Bullet object. Erasing vector does only remove the pointers, not delete the pointed to object.

Your second example does properly delete Bullets (not all of them given the conditionals and loop setup), but only if you do the necessary deletion at all points where the vector is modified and manually assure that a pointer is never deleted twice (e.g. because it is present in two vectors). This is not how containers are supposed to be used.

If you really want to store pointers and have the vector own them, then you should use std::vector<std::unique_ptr<Bullet>> which will properly delete the Bullet objects when the vector is erased. However the vector will become non-copyable (only movable).

If however the vector is supposed own the Bullets anyway, then you can simply store them directly in the vector: std::vector<Bullet>

If the vector is not supposed to own the Bullets then the duty to delete them lies with whatever object is actually owning them, but even then there needs to be some effort made to assure deleted pointers do not remain in the vector.

Answer 2

You need to delete the objects you've created with new (unless you surrender the pointer to a std::unique_ptr or std::shared_ptr). Consider letting the vector be the owner of the objects instead.

std::vector<Bullet> bulletVector;

// in C++17, emplace_back returns a ref to the new Bullet:
Bullet& aNewBullet = bulletVector.emplace_back(x,y,dirX,dirY,size,duration);

// pre C++17:
bulletVector.emplace_back(x,y,dirX,dirY,size,duration);
Bullet& aNewBullet = bulletVector.back();

aNewBullet.assignTexture(btext);

When you now erase() an object from the vector, it will be destructed automatically.

Answer 3

First, it is highly likely you problem is solvable by not using manual dynamic allocation in the first place. There are reasons where you can use dynamic allocation (a polymorphic storage, for example), but even then you should steer toward smart pointers rather than manual allocations.

Ditch the pointers

To do this with a regular vector of concrete objects (not pointers), you would simply

std::vector<Bullet> bulletVector;

Additions could be done with something like this:

bulletVector.emplace_back(x,y,dirX,dirY,size,duration);
bulletVector.back().assignTexture(btext);

Removing all elements based on duration reaching zero would simply use the remove/erase idiom like this:

bulletVector.erase(std::remove_if(
                    bulletVector.begin(), 
                    bulletVector.end(),
                    [](auto const& b) { return b.getDuration() == 0; }),
                   bulletVector.end());

Keep in mind, while this is easily the preferred method for storing these things, it will require changes in the rest of your code. Everywhere you currently do this:

bulletVector[i]->doSomething();

will become

bulletVector[i].doSomething();

---

More Intelligent Pointers

If you must use dynamic allocation, don't use manually managed pointers. They're a recipe for memory leaks and ownership responsibility, and if you think it can't happen to you, you're being naive. Rather, use smart pointers. The two main smart pointer templates are std::unique_ptr and std::shared_ptr. There is also std::weak_ptr, which deserve an honorable mention, as it comes in very handy in certain situations (not yours, that I can tell).

std::vector<std::shared_ptr<Bullet>> bulletVector;

Hence additions can be done like this (assuming this is done with polymorphism in mind and there may be more than one bullet type, in this case MagicBullet and SilverBullet, both derived from Bullet):

std::shared_ptr<Bullet> bullet = std::make_shared<MagicBullet>(x,y,dirX,dirY,size,duration);
bullet->assignTexture(btext);
bulletVector.emplace_back(bullet);

bullet = std::make_shared<SilverBullet>(x,y,dirX,dirY,size,duration);
bullet->assignTexture(btext);
bulletVector.emplace_back(bullet);

or just store regular bullets:

bullet = std::make_shared<Bullet>(x,y,dirX,dirY,size,duration);
bullet->assignTexture(btext);
bulletVector.emplace_back(bullet);

The removal algorithm is identical, only the condition predicate function changes (and not by much)

bulletVector.erase(std::remove_if(
                    bulletVector.begin(), 
                    bulletVector.end(),
                    [](auto const& b) { return b->getDuration() == 0; }),
                   bulletVector.end());

The nicest part of this is the remainder of your code will likely require little to no changes whatsoever. But I stress, that's the only real benefit (apart form the obvious, namely not having to manage the memory yourself any longer).

---

Given the choice between the two methods (vector of concrete objects vs vector of smart pointers) choose carefully. If the former works, use it. If it doesn't, either think about how it could work, or use the latter.

But above all, stop managing memory manually.

Answer 4

Of course, you must deleting Bullets by yourself by delete. But your deletion cycle is not correct.

To delete values from vector with some condition, use something like this:

auto condFunc = [](const Bullet* x)
{
    bool rez = (x->getDuration() == 0);
    if(rez)
        delete x;
    return rez;
};
bulletVector.erase(std::remove_if(bulletVector.begin(), bulletVector.end(), condFunc), bulletVector.end());