How to properly clean up elements from vectors of object pointers

Question

I have been studying dynamic allocation and came across this question on StackOverflow:

Deallocating objects stored in a vector?

One of the upvoted answers explain how to manually manage memory when using "Vectors of Pointers to Objects": iterating through the vector calling delete.

My question is about how to delete particular elements of a vector, not the whole vector. In the example program below, I have a vector of object pointers. Imagine that the x variable of these objects are decremented over time... When the x value of an object reaches a number (let's say 3), I wish to delete the object; HOWEVER, I would like to keep the vector SORTABLE by the x values of the objects at all times.

The problem is that when I call delete on the objects whose x values reach 3, the object is deleted but there's still a pointer there pointing to random memory locations, and the size of the vector stays the same as well.

When I loop through the vector printing the x values, the elements that I called delete on are still there but pointing to values like -53408995. How do I get rid of the pointer element of the vector as well as the object?

Calling erase is not an option because in my actual program (not the minimal example below) the vector is continuously SORTED by other factors that change the x-value equivalents. I can't keep track of their index. I would like to delete both the object and the pointer element when I iterate through the vector to check for the x value.

Example:

#include <iostream>
#include <vector>

class A
{
public:
    A(int i) { x = i; }
    int x;
};

int main()
{
    std::vector<A*> Vec;

    Vec.push_back(new A{ 5 });
    Vec.push_back(new A{ 4 });
    Vec.push_back(new A{ 3 });

    std::cout << "Size before = " << Vec.size() << std::endl; // 3

    for (auto& a : Vec)
    {
        std::cout << a->x << std::endl;

        if (a->x == 3) { delete a; }
    }

    std::cout << "Size after = " << Vec.size() << std::endl; // Still 3!

    for (auto& a : Vec)
    {
        std::cout << a->x << std::endl; // Prints 5, 4 and a random memory location like -34528374
    }

    return 0;
}

Change: `std::vector Vec;` to `std::vector> Vec;` job done. Or ask yourself why it is not just `std::vector Vec;` — Richard Critten, May 04 '18 at 13:14
Well, almost job done; replace all the `new A` with `std::make_unique` would be better — UKMonkey, May 04 '18 at 13:15
The vector is also supposed to hold pointers to derived classes of A. The objects also need to have pointers to them in various other vectors/maps. — Nathaniel G.M., May 04 '18 at 13:18
I have also tried using a vector of shared pointers, but I still have the same problem. I need to get rid of the pointer (without knowing its index) and the object at the same time. — Nathaniel G.M., May 04 '18 at 13:36
@NathanielG.M. `remove_if`ing a `unique_ptr` from the vector destroys the object it points to. `remove_if`ing a `shared_ptr` from the vector destroys the object if it is the last `shared_ptr` pointing to that object — Caleth, May 04 '18 at 14:49

score 3 · Accepted Answer · answered May 04 '18 at 14:40

you mentioned in the comments that the objects will have other pointers to them. this sounds like std::shared_ptr<A> to me. this way you can have other pointers to your A objects without memory leaking issues. (std::shared_ptr comes with a small(!) performance cost, but you should not worry about it for now). additionally i changed your passage to delete/erase your element from the vector. be aware that the A objects are still alive IF there are other instances keeping a std::shared_ptr<A> copy to it (but thats a good thing).

here is the code:

#include <iostream>
#include <vector>
#include <memory>
#include <algorithm>

class A
{
public:
    A(int i) { x = i; }
    int x;
};

int main()
{
    std::vector<std::shared_ptr<A>> Vec;
    Vec.emplace_back(std::make_shared<A>(5));
    Vec.emplace_back(std::make_shared<A>(4));
    Vec.emplace_back(std::make_shared<A>(3));

    std::cout << "Size before = " << Vec.size() << std::endl;

    Vec.erase(
        std::remove_if(std::begin(Vec),std::end(Vec), [](auto&& ptr){ return ptr->x == 3;}),
        std::end(Vec));

    std::cout << "Size after = " << Vec.size() << std::endl;
    for (auto&& a : Vec)
    {
        std::cout << a->x << std::endl;
    }

    return 0;
}

Iterating through the vector an extra time just to call erase was something I wanted to avoid, however if there is simply no other way then I'll accept this as the answer and use the lambda you wrote. Thank you. — Nathaniel G.M., May 04 '18 at 15:00
its not something you have to worry about. you are not plain iterating 2 times over the vector. remove_if reorders your objects, so that all elements you want to delete are in the end of the vector. erase just takes this range and destorys the objects and resizes the vector accordingly. this work has to be done anyway, so there is no performance penalty you pay you would not otherwise — phön, May 04 '18 at 15:16

marouane18 · Answer 2 · 2018-05-04T15:17:33.730

in this case you have to use an std::list container

#include <iostream>
#include <list>

class A
{
public:
    A(int i) { x = i; }
    int x;
};

int main()
{
    std::list<A*> Vec;

    Vec.push_back(new A{ 5 });
    Vec.push_back(new A{ 4 });
    Vec.push_back(new A{ 3 });

    std::cout << "Size before = " << Vec.size() << std::endl;

    for (auto& a : Vec)
    {
        std::cout << a->x << std::endl;
    }

    Vec.remove_if([](A* a){
        bool status = (a->x == 3);
        if(status){
            delete a;
            return true;
        }else{
            return false;
        }
    });

    std::cout << "Size after = " << Vec.size() << std::endl;

    for (auto& a : Vec)
    {
        std::cout << a->x << std::endl;
    }

    return 0;
}

output :

Size before = 3
5
4
3
Size after = 2
5
4

I rewrite your code adding some improvements

#include <iostream>
#include <list>

class A
{
public:
    A(const int& i):x(i) {} // so X is assigned to i in construction ( more efficient )
    int get_x() const {return x;}
private:
    int x; // x have to be private ( good practice )
};

int main()
{
    std::list<A> List; // A instead of A* so the process of construction / destruction is handled automatically

    List.emplace_back(5); // append element and constructed at the same time
    List.emplace_back(4); // see std::list for more details
    List.emplace_back(3);

    std::cout << "Size before = " << List.size() << std::endl;

    for (auto& a : List)
    {
        std::cout << a.get_x() << std::endl;
    }

    List.remove_if([](A a){ return a.get_x() == 3;});

    std::cout << "Size after = " << List.size() << std::endl;

    for (auto& a : List)
    {
        std::cout << a.get_x() << std::endl;
    }
    return 0;
}

Why have you named a `std::list` Vec? – Caleth May 04 '18 at 14:42 — Caleth, May 04 '18 at 14:42
i just took the same name as the code in the question. – marouane18 May 04 '18 at 15:15 — marouane18, May 04 '18 at 15:15
i had just change it to List, thank you for your remark :) – marouane18 May 04 '18 at 15:18 — marouane18, May 04 '18 at 15:18

score 1 · Answer 3 · answered May 04 '18 at 13:55

1

std::vector<std::unique_ptr<A>> vec;

This would handle the normal deletion and exit through exception.

answered May 04 '18 at 13:55

Archie Yalakki

512
4
12

this might work, but in my opinion it's not the right way to do things in this context – marouane18 May 04 '18 at 14:02

score 0 · Answer 4 · answered May 05 '18 at 18:32

After studying iterators a bit, I also came up with an answer that uses raw pointers:

for (std::vector<A*>::iterator it = Vec1.begin(); it != Vec1.end(); )
{
    if ((*it)->x == 3)
    {
        delete * it;
        it = Vec1.erase(it);
    }
    else 
    {
        ++it;
    }
}

I will keep phön's post as the answer as smart pointers should always be preferred over raw pointers if available.

How to properly clean up elements from vectors of object pointers

4 Answers4