Singleton class and correct way to access it in C++

Question

I'm currently trying to implement the singleton pattern in C++. After reading about it for a while now, it seems like there are a really large amount of different ways to do this.

Is the way I ended up doing it correct or did I miss out on something important here? Is this implementation thread safe in modern C++ (I read there were some changes to that in particular with C++11)? In addition I am not sure at all about the way I am accessing the class within MyFunction().

Is it really necessary to use a raw pointer there? Does this code contain any memory leaks?

#include <iostream>

class Singleton
{
private:

    Singleton();
    ~Singleton();  

public:

    static Singleton& instance()
    {
        static Singleton INSTANCE;
        return INSTANCE;
    }

    void Test();
};

void Singleton::Test()
{
    std::cout << "Test() called" << std::endl;
}

Singleton::Singleton()
{
    std::cout << "CONSTRUCTOR CALLED" << std::endl;
}

Singleton::~Singleton()
{
    std::cout << "DESTRUCTOR CALLED" << std::endl;
}

void MyFunction()
{
    // use the singleton class
    Singleton * MySingleton = &Singleton::instance();   
    MySingleton->Test();    
}


int main()
{
    MyFunction();
    return 0;
}

Answer 1

Thread safety

Accessing the singleton is thread-safe. The change in C++11 has forced compilers to implement the construction of local static variables (like INSTANCE) in a thread-safe manner.

Note, however, that this doesn't make Singleton thread-safe under all circumstances: If multiple threads are calling Singleton::Test, I wouldn't be surprised to see some garbled output, as access to std::cout isn't synchronized.

Correctness

The current implementation violates the rule of 5: It has a custom destructor, but no custom copy and move constructors and assignment operators. And, to prevent making a copy of the singleton instance (or moving from it!), these 4 special functions should be deleted, like so:

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

Note the public specifier, this ensures that these deletions are visible everywhere needed.

Access

While taking a pointer from the reference returned by Singleton::instance() is a valid use, it seems unnecessary in MyFunction as the pointer isn't required. Why not simply capture by a reference, like so:

auto& MySingleton = Singleton::instance();
MySingleton.Test();

Or if only one call is done:

Singleton::instance().Test();

Answer 2

You have created the classic Singleton pattern for C++.
https://stackoverflow.com/a/1008289/14065

This is great as a learning exercise. But in real life you can't use the Singleton like that because it tightly couples the objects to the things that use it. For instance testing your code now becomes exceeedingly difficult.

To use a s Singleton well (so it is not considered an anti pattern) you also need to combine the Singletons with another builder pattern (most will work but factory is easiest).
https://softwareengineering.stackexchange.com/a/40374/12917

You may have a default version of a Singleton that is run in production. But you should be able to configure the system so that at runtime the Singleton that is built at run-time is dependent on situation (so for testing you always insatiate an appropriate test Singleton).

Code Review:

Copying

You should prevent copying/moving of the object. If you allow copying it is no longer a Singleton, if you allow moving then it is it valid? Disable the copy/move constructors and assignment operators.

Usage

    // No reason to use a pointer.
    Singleton * MySingleton = &Singleton::instance();

    // When you have a pointer there is no ownership semantics
    // associated. So it is easy to make a mistake and accidentally
    // call delete on the pointer.


    // It returns a reference, so capture a reference
    Singleton&  MySingleton = Singleton::instance(); 

    // Here ownership semantics are clear.
    // You don't own the object (you simply have a reference to it).
    // It is your responsibility to make sure the object has not been
    // destroyed since you retrieved the reference.

    // Thus usually easier to use a Singleton as soon as you get it.
    Singleton::instance().Test();   // Don't need to keep a ref.

Answer 3

The way you have it will initialize the instance the first time it is used, if ever. That is handy for a component that may or may not be needed, and as an attempt to get dependencies between such systems automatically correct.

It will be destroyed during program shut down, after main returns (so, no memory leak), in the reverse order that such scoped static variables were created. If it gets used after that, you will have problems. Some singletons such as a logger may very well get used that way. You have no way to trap such bad uses, which is the downside of letting the compiler handle the details of creation and destruction.

---

⧺Enum.5 Don't use ALL_CAPS for enumerators and ⧺ES.9 Avoid ALL_CAPS names.