How to remove a Pointer to an Object in a list of Pointers in the class Destructor?(C++)

[-]

mredding@reddit

This shouldn't be done, because it can't be done without violating higher programming principles.

RAII - Resource Acquisition Is Initialization. In other words - constructors aren't factories. A class models behaviors by maintaining an invariant - statements about the class that must always be true when the class is observed. When control is handed over to a class, it may suspend its invariant, but it must be reestablished before returning control to the caller.

What does this look like in practice? A standard vector is implemented in terms of 3 pointers. These pointers have a relationship, and that relationship must ALWAYS be valid. The vector can suspend the invariant to reallocate, like when you call push_back, but it's reestablished before returning - even if the call throws an exception.

NEVER is the class in an indeterminate or intermediate state. When the ctor returns, the class is valid. There is no separate init function to call, because WTF are you going to do with an object between construction and initialization, but just call init? All the configuration should be done by a factory creating the component member objects, and then passing them to the constructor of some composite object.

What this means is that a constructor invariant must be established by the end of the initializer list. You can suspend the invariant in the ctor body, if you need, but you should never enter the ctor body in an invalid state. There's no language level enforcement, this is just how you should do it. Ctors aren't typically supposed to allocate their own resources, they should be taking ownership of resources passed to them. This isn't a strict rule, but it's a damn good guideline.

So an object adding itself to a global list is a tough nugget to solve. You need an index or an iterator to track the object itself. You can't initialize the field in the initializer list because push_back doesn't return anything. You can't initialize it after a push_back in the ctor body, because you're already past the initializer list, you're violating the guideline by entering the body invalid. Also, the act of being a member of the global list itself is invariant to the class - IF the class is to maintain this invariant relationship itself; this is a cyclic invariant that cannot satisfy itself. Also consider adding yourself to the list and then throwing in the ctor; the dtor isn't called. So now you have a dangling pointer you can't get rid of and no way of reliably identifying. Comparing invalid pointers is UB - and there's no fucking arguing with that; if you do it, it's just wrong.

Then after all that, come destruction, there's NO way of knowing who is holding a reference to that pointer. You can use a mutex to guard the global list, but you would have to build guards around that access to make sure its enforced, and that doesn't save anyone from a dangling reference. Removing elements from a vector also comes with a significant computational cost if done naively - you have to shift all subsequent elements. And no matter what, you're changing the indexing of objects, if you thought that was a good way to separate a reference from a dangling pointer. Index i was supposed to be a health item, but now it's a BFG.

And then what does the dtor do if it's iterator is invalidated? If the objects are orphaned that way, because the list was externally cleared, you can't compare an invalid iterators. But how would you know? Can't rely on indexes, those can get out of sync as mentioned before.

So then the only way to make it work safely is if you build a bigger registry object that strictly coordinates object insertion, lifetime management, and access. This can quickly turn into a lot of machinery. I don't know how exactly it would be built - you'd want some sort of transactional object that can attempt to add to the list but revert if not committed - like with an exception.

I laid out a FAIR number of invariants and constraints above that you would have to hold/prevent. All this should be screaming bad design to you. With a few years of practice, you'll learn to sense this stuff and trust your intuition.

Yes, you can code a fairly straight-forward implementation of the behavior you want, but all these problems would exist. It wouldn't be easy to reason about the code, and it couldn't be made safe. You would rely HEAVILY on developers doing the Right Thing(tm) and following all these conventions ad-hoc, by themselves, on their honor - as though they could know it all and get it all right all of the time... And I'm talking about you - go ahead and try it: go one day without screwing this up. Right? How about 6 months from now?

I would first try to store object by value instead of pointer. You don't need to heap allocate your objects, they're already on the heap by virtue of the container being dynamic.

To help you out there, there are MORE types of polymorphism in C++ than just late binding. You PROBABLY don't want a virtual base class - you probably want a variant, even if all the types are a part of the same hierarchy.

class game_object {};

class mobile: public game_object {};
class player: public mobile {};
class enemy: public mobile {};
class vehicle: public mobile {};

class item: public game_object {};
class weapon: public item {};
class buff: public item {};

using game_elements = std::variant<player, enemy, vehicle, item *>;

See? You can mix and match between static and dynamic polymorphism. This helps you where you don't have to leak derived class specifics into the base class - that's how you end up with a bunch of virtual methods that no-op because depending on where in the hierarchy - items aren't themselves mobile, right?

There's more thinking that can go into this. Stuff for you to think about.

[-]

Tatt00ey@reddit

Erase remove is the safe way. But honestly, having objects register themselves in a global vector is a ticking clock. Consider a scene graph or manager class that owns and destroys everything in one place. Less risk of dangling pointers when objects get copied or moved unexpectedly. Future you will thank you.

[-]

WishboneComplete3410@reddit

You can remove it with the erase/remove pattern (std::erase in C++20 is even nicer), but I’d be more worried about the design here. A global-ish list of raw this pointers gets weird fast once objects are copied/moved. If you can, let one container own the objects and iterate that, instead of every object registering itself.

[-]

Zestyclose-Window358@reddit (OP)

sorry but what do you mean by letting one container own the Objects?

[-]

TheSkiGeek@reddit

Something like your global list being a std::vector>.

[-]

crisp_lynx_370@reddit

the global inline vector approach works but its gonna bite you eventually, having objects register themselves on construction like that makes ownership really unclear. worth looking into a proper scene/world class to manage their lifetime instead

[-]

Bitter-Ad-3803@reddit

which c++ version are you using

[-]

Zestyclose-Window358@reddit (OP)

C++ 17

[-]

Big-Rub9545@reddit

Or otherwise .find() + .erase().

[-]

crisp_lynx_370@reddit

the global inline vector approach works but its gonna bite you eventually, having objects register themselves on construction like that makes ownership really unclear. worth looking into a proper scene/world class to manage their lifetime instead

[-]

blackneckbean@reddit

std::erase(GameObjects, this); if you’re on C++20.

Otherwise classic erase-remove:

GameObjects.erase(
    std::remove(GameObjects.begin(), GameObjects.end(), this),
    GameObjects.end()
);

But honestly this pattern gets risky fast with raw pointers/object lifetime.

[-]

Zestyclose-Window358@reddit (OP)

Can you Elaborate on why it would be risky?i know global anything is bad,but i was planning on using something to avoid DRY until i run into a problem with this design choice.

thanks.

[-]

WystanH@reddit

Your child should be able to kill itself.

Here's a quick, minimal, test:

#include <iostream>
#include <string>
#include <vector>
#include <algorithm> // Required for std::find

class Thing;

inline std::vector<Thing*> Things;

class Thing {
public:
    Thing(const std::string &n) : name(n) {
        std::cout << *this << " created" << std::endl;
        Things.push_back(this);
    }
    ~Thing() {
        std::cout << *this << " destroyed" << std::endl;
        auto it = std::find(Things.begin(), Things.end(), this);
        if (it != Things.end()) {
            std::cout << "Found: " << *it << std::endl;
            Things.erase(it);
        } else {
            std::cout << "Not found" << std::endl;
        }
    }

    friend std::ostream& operator<<(std::ostream& os, const Thing& x) {
        return os << "Thing(" << x.name << ")";
    }    
private: 
    std::string name;
};

void showThingCount() {std::cout << "Things(" << Things.size() << ")" << std::endl; }

int main() {
    showThingCount();
    {
        Thing a("A");
        showThingCount();
    }
    std::cout << "First Thing should be dead." << std::endl;
    showThingCount();
    return 0;
}

Output:

Things(0)
Thing(A) created
Things(1)
Thing(A) destroyed
Found: 0x505470
First Thing should be dead.
Things(0)

That said, I don't like std::vector<GameObject*> out there global and raw. If you make a class for it, then any related utility function can belong to that class. That allows you to move all that code out of the child, at least.

e.g.

#include <iostream>
#include <string>
#include <vector>
#include <algorithm> // Required for std::find

class Thing;

class ThingContainer {
private:
    std::vector<const Thing*> items;
public:
    size_t size() const { return items.size(); }
    void register_thing(const Thing *x) { items.push_back(x); }
    void unregister_thing(const Thing *x) { 
        auto it = std::find(items.begin(), items.end(), x);
        if (it != items.end()) {
            std::cout << "Found: " << *it << std::endl;
            items.erase(it);
        } else {
            std::cout << "Not found" << std::endl;
        }
    }

    friend std::ostream& operator<<(std::ostream& os, const ThingContainer& x) {
        return os << "Things(" << x.size() << ")";
    }    
};

inline ThingContainer Things;

class Thing {
public:
    Thing(const std::string &n) : name(n) {
        std::cout << *this << " created" << std::endl;
        Things.register_thing(this);
    }
    ~Thing() {
        std::cout << *this << " destroyed" << std::endl;
        Things.unregister_thing(this);
    }

    friend std::ostream& operator<<(std::ostream& os, const Thing& x) {
        return os << "Thing(" << x.name << ")";
    }    
private: 
    std::string name;
};

void showThingCount() {std::cout << Things << std::endl; }

int main() {
    showThingCount();
    {
        Thing a("A");
        showThingCount();
    }
    std::cout << "First Thing should be dead." << std::endl;
    showThingCount();
    return 0;
}