Automatically adding and removing an object from a list

Question

I have a class. When this class is instantiated, I want the instance added to a list. When the object is deleted, I want it removed from the list.

So I give the object a shared pointer to itself. I then have a list of weak pointers to those shared pointers. When an object is created, it creates a shared pointer to itself, makes a weak pointer to that, and puts the weak pointer in a list.

When the object is destroyed, the shared pointer is as well. Whenever I try to access a member in the list, I ensure that it hasn't expired and that its use count isn't 0. Despite this, I still crash when the list member is destroyed. Why? Can I get around it? Here's my SSCCE:

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

class test
{
    private:
        std::shared_ptr<test> self;

    public:
        int val;
        test(int set);

        test(test &copy) = delete; // making sure there weren't issues 
                                   // with a wrong instance being deleted
};

std::vector<std::weak_ptr<test>> tests;

test::test(int set):
    val(set)
{
    this->self = std::shared_ptr<test>(this);

    tests.push_back(std::weak_ptr<test>(this->self));
}

void printTests()
{
    for (auto i = tests.begin(); i != tests.end(); i++)
    {
        if (i->use_count() == 0 || i->expired())
        {
            tests.erase(i);
            continue;
        }

        std::cout << i->lock()->val << std::endl;
    }

    std::cout << std::endl;
}

int main(int argc, char **argv)
{
    {
        test t(3);

        std::cout << "First tests printing: " << std::endl;

        printTests();
    } // SEGFAULTS HERE

    std::cout << "Second tests printing: " << std::endl;
    printTests();

    return 0;
}

The output of this program is as follows:

First tests printing:
3

Segmentation fault (core dumped)

Answer 1

Your issue is with how you are creating the self pointer:

 this->self = std::shared_ptr<test>(this);

When a shared_ptr is created with this constructor, according to the documentation,

When T is not an array type, constructs a shared_ptr that owns the pointer p. ... p must be a pointer to an object that was allocated via a C++ new expression or be 0

So the issue is that the shared_ptr is taking ownership of your stack object, so when the object gets destructed (and the shared_ptr along with it), shared_ptr is trying to delete your object that is on the stack. This is not valid.

For your use case, if you expect tests to outlive your vector, then you might be able to just store this.

Answer 2

I think the OP is interested in a solution to his original problem even if it uses a different method than the one he attempted. Here is a simple example of how to add an object to a global list when it is constructed, and remove it when it is deleted. One thing to remember: you must call AddList in every constructor you add to your base class. I didn't know whether you want the list to be accessible outside the class or not, so I added getter functions to return non-const iterators to the list.

class MyClass
{
private:
    static std::list<MyClass*> mylist;
    std::list<MyClass*>::iterator mylink;

    // disable copy constructor and assignment operator
    MyClass(const MyClass& other);
    MyClass& operator = (const MyClass& other);

    void AddList()
    {
        mylink = mylist.insert(mylist.end(), this);
    }

    void RemoveList()
    {
        mylist.erase(mylink);
    }

public:
    MyClass()
    {
        AddList();
    }

    virtual ~MyClass()
    {
        RemoveList();
    }

    static std::list<MyClass*>::iterator GetAllObjects_Begin()
    {
        return mylist.begin();
    }

    static std::list<MyClass*>::iterator GetAllObjects_End()
    {
        return mylist.end();
    }

    virtual std::string ToString() const
    {
        return "MyClass";
    }
};

class Derived : public MyClass
{
    virtual std::string ToString() const
    {
        return "Derived";
    }
};

std::list<MyClass*> MyClass::mylist;


int main()
{
    std::vector<MyClass*> objects;
    objects.push_back(new MyClass);
    objects.push_back(new MyClass);
    objects.push_back(new Derived);
    objects.push_back(new MyClass);

    for (std::list<MyClass*>::const_iterator it = MyClass::GetAllObjects_Begin(), end_it = MyClass::GetAllObjects_End(); it != end_it; ++it)
    {
        const MyClass& obj = **it;
        std::cout << obj.ToString() << "\n";
    }

    while (! objects.empty())
    {
        delete objects.back();
        objects.pop_back();
    }
}

Answer 3

This line is trouble:

tests.erase(i);

An iterator pointing to the erased element is invalid, and you can't increment it any longer. Luckily, erase returns a new iterator you can use:

auto i = tests.begin();
while (i != tests.end())
{
    if (i->use_count() == 0 || i->expired())
    {
        i = tests.erase(i);
    }
    else {
        std::cout << i->lock()->val << std::endl;
        ++i;
    }
}