Singleton rewrite based on old one

Question

I inhereted an implementation of a singleton template. Althoug I read that singletons are bad, my program uses singletons throughout and I can not get rid of them at the moment.

This is the implementation I started with:

template <typename T>
class Singleton
{
public:
    static T* ms_singleton;

    Singleton()
    {
        assert(!ms_singleton);
        long offset = (long)(T*) 1 - (long)(Singleton <T>*)(T*) 1;
        ms_singleton = (T*)((long) this + offset);
    }

    virtual ~Singleton()
    {
        assert(ms_singleton);
        ms_singleton = 0;
    }

    static T &instance()
    {
        assert(ms_singleton);
        return (*ms_singleton);
    }

    static T &Instance()
    {
        assert(ms_singleton);
        return (*ms_singleton);
    }

    static T* instance_ptr()
    {   return (ms_singleton); }
};

template <typename T>
T* Singleton <T>::ms_singleton = NULL;

And the new implementation.

#pragma once

template<typename T>
class Singleton
{
public:
    static T &Instance()
    {
        static T* myInstance;
        return (*myInstance);
    }

    Singleton(Singleton const &) = delete;
    Singleton(Singleton &&) = delete;
    Singleton &operator=(Singleton const &) = delete;
    Singleton &operator=(Singleton &&) = delete;

protected:
    Singleton() = default;
    virtual ~Singleton() = default;
};

The first code doesn't create the class and only sets a pointer to an existing instance; the new implementation does the same.

Do you think the second implementation is correct? Is there something that should be optimized or changed? There may be problems if I use it like that? Should I also handle the creation of classes?

This new implementation should behave the same as first, because I have a big project and I just can't get rid of this singleton.

How good is the new singleton?

Answer 1

Toby Speight's answer is correct for a normal singleton, but the class that you're replacing doesn't look like a normal singleton.

The original class appears to allow the user to explicitly define the object lifetime. The static variable is actually just for ease of access to that object:

• The new code changes the point of construction to the first call to Instance().
• The point of destruction is changed from reverse-order of construction inside the owning scope, to reverse-order of construction when C++ cleans up static variables.

This may or may not matter to your use case, but note that these two factors combined can completely alter the order of destruction of your "singleton" objects.

If you want to keep managing the object lifetimes explicitly, it might be better to use something like the Service Locator pattern instead (which is closer to what the original code does).

---

A related example from my own code (the shared / weak ptrs could be replaced with raw pointers or some sort of handle):

template<class T>
class Service
{
public:

    static void Provide(std::weak_ptr<T> service)
    {
        if (Service::service.lock())
            throw std::runtime_error("Service is already being provided!");

        Service::service = service;
    }

    static void Stop()
    {
        Service::service = std::weak_ptr<T>();
    }

    static T& Get()
    {
        auto strongService = service.lock();

        if (!strongService)
            throw std::runtime_error("Attempting to use a null service!");

        return *strongService;
    }

private:

    Service() = delete;

    static std::weak_ptr<T> service;
};

template<class T>
std::weak_ptr<T> Service<T>::service;

Answer 2

The second implementation looks complete but wrong - Instance() dereferences an uninitialized pointer. You probably meant:

static T &Instance()
{
    static T myInstance;
    return myInstance;
}

---

There's no need to delete the move construction and assignment - deleting the copy construction and assignment will prevent the move versions being automatically generated. (But you might prefer what you have as being clearer to future users).

---

There's certainly no need for a virtual destructor, or even a constructor - the only useful member is the static Instance().