3
\$\begingroup\$

In my project, I need to be able to iterate through all living instances of a given class. I ran into a problem regarding static initialization/deinitization order where when an instance attempts to remove itself from the registry (in its destructor) when the program exists, there's a chance that the registry will have already been popped off of the stack (e.g., smart pointers would end up calling the destructor too late if they were initialized before the registry). This is my attempt at getting around that:

// InstanceRegistry.h

#pragma once
#include <unordered_set>

// Forward declarations

template <typename T>
class InstanceRegisterableBase;

// Stores a collection of pointers to all living instances of the given class. The class must inherit from InstanceRegisterableBase.
template <typename T>
class InstanceRegistry {

    // Used to detect the imminent destruction of the instance collection.
    struct DestructionHandler {

        ~DestructionHandler() {

            // Set removal flag to false for all instances so that they do not attempt to remove themselves after the instance collection has been freed.
            for (auto i = InstanceRegistry<T>::Registry().begin(); i != InstanceRegistry<T>::Registry().end(); ++i)
                (*i)->_do_remove = false;

        }

    };

public:
    // Adds a instance to the registry. Returns false if the instance already exists in the registry.
    static void Add(InstanceRegisterableBase<T>* ptr) {

        _Registry().insert(ptr);

        // Make sure that the destruction handler is pushed onto the stack after the registry collection.
        // This ensures that it will be popped off first to act as a warning that the registry collection is about to be lost.
        int dummy = (_destruction_handler, 0);

    }
    // Removes an instance from the registry. Returns false if the instance does not exist in the registry.
    static void Remove(InstanceRegisterableBase<T>* ptr) {

        _Registry().erase(ptr);

    }
    // Returns a collection containing all instances.
    static const std::unordered_set<InstanceRegisterableBase<T>*>& Registry() {

        return _Registry();

    }

protected:
    static std::unordered_set<InstanceRegisterableBase<T>*>& _Registry() {

        return  _registry;

    }

private:
    static std::unordered_set<InstanceRegisterableBase<T>*> _registry;
    static DestructionHandler _destruction_handler;

};

// Classes that inherit from this class can be added to an instance registry. 
template <typename T>
class InstanceRegisterableBase {
    friend class InstanceRegistry<T>;

public:
    InstanceRegisterableBase() {

        // Add the instance to the registry.
        InstanceRegistry<T>::Add(this);

        // By default, the instance should be removed from the registry on destruction.
        _do_remove = true;

    }
    virtual ~InstanceRegisterableBase() {

        // Remove the instance from the registry if the registry is still valid.
        if (_do_remove)
            InstanceRegistry<T>::Remove(this);

    }

private:
    bool _do_remove;

};

// Static member initialization
template <typename T>
std::unordered_set<InstanceRegisterableBase<T>*> InstanceRegistry<T>::_registry;
template <typename T>
typename InstanceRegistry<T>::DestructionHandler InstanceRegistry<T>::_destruction_handler;

To use it, the class needs to inherit from InstanceRegisterableBase. Then, in its constructor, it will be added to the appropriate registry, and upon destruction, it will be removed.

To get around the initialization/deinitialization order issue, I have a static instance of DestructionHandler that gets pushed onto the stack after the registry collection (an unordered_set). That way, the instance of DestructionHandler is popped off of the stack first, and can tell all living instances of InstanceRegisterableBase not to try to remove themselves from the collection (because it's about to be deinitialized).

It seems to be working nicely, I'm just worried I might be relying on undefined behavior. As I understand, static variables in the same translation unit will be initialized in the order they're listed. Therefore, I listed DestructionHandler last, so it would be popped off first.

Another (minor) concern: I'm not quite happy with the class names. I feel like "registry" might be the wrong word, but I'm not sure what else to call it.

All-in-all, is this an okay design, or am I totally off the mark in my approach to this problem?

(PS: Some of the comments say that the functions should return a value, which contradicts with the return type of void. Please disregard any inconsistencies in this regard.)

\$\endgroup\$
1
\$\begingroup\$

Naming

Names beginning with an underscore followed immediately are reserved to the implementation for any purpose (which means they might be defined as macros). Personally, I always avoid leading underscores in identifiers, as that's easier than learning the detailed rules. Even if you do know the rules, _Registry() is a complete no-no.

Thread safety

None of the accesses to the std::unordered_set have any locking, so it's up to the calling to to arrange that. Difficult at the best of times, but close to impossible for destructors. I think that InstanceRegistry requires a std::mutex that can be obtained using a std::lock_guard (or perhaps std:scoped_lock in C++17) in each of the registry methods.

Avoid the need to inherit

We already have a mechanism for performing actions when an object is deleted, and that is the smart pointer. If you're able to insist that your objects are always heap-allocated, then you may be able to get the same effect for much less work by writing a factory method that creates and registers objects, returning them wrapped in a smart pointer with a deregistering deleter. The deleter simply removes the object's registration before invoking std::default_deleter<T>() (or perhaps your preferred custom deleter).

Another way to be able to register objects whose base classes you don't control is to use the Curiously Recurring Template Pattern (CRTP) to add the functionality in the subclass rather than the base class.

I'm not saying that either of the above are necessarily improvements, but at least be aware that they are options.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Ugh, I got so used to using a single underscore for private member variables that I forgot that an underscore followed by a capital letter is reserved. It just goes to show that even if you are aware of the rules, it really is easy to slip up... I definitely agree with the "you may be able to get the same effect for much less work". After some thought I think I'm way over-complicating things with this design. I appreciate the feedback; I'll take note of it as I think over this problem again. \$\endgroup\$ – Squidy Jul 31 '17 at 11:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.