Modern C++ Singleton Template

Question

I recently read about the C++17 static inline member declaration and thought that this will make templates a little bit cleaner, since static members can now be initialized inside a templated class.

Because of this I wanted to create a neat little Singleton template (since it is the perfect example where static members are needed).

Now to my questions: Is there something I potentially missed, i.e. are there possibilities to create copies of a derived Singleton? Is it a good idea to use CRTP for a Singleton in general? What a about the move constructor, do I need to handle it as well?

Here is the template:

template < typename T >
class Singleton {
 public:
 static T& GetInstance() {
 static MemGuard g; // clean up on program end
 if (!m_instance) {
 m_instance = new T(); 
 }
 return *m_instance;
 }
 Singleton(const Singleton&) = delete;
 Singleton& operator= (const Singleton) = delete;
 protected:
 Singleton() { };
 virtual ~Singleton() { }
 private:
 inline static T * m_instance = nullptr;
 class MemGuard {
 public: 
 ~MemGuard() {
 delete m_instance;
 m_instance = nullptr;
 }
 };
};

And here a possible derived type:

class Test final : public Singleton<Test> {
 friend class Singleton<Test>;
 public:
 void TestIt() { };
 private:
 Test() {} 
 ~Test() { /* Test intern clean up */ }
};

• Tom Tanner: From my experience, it's a good idea not to have singletons at all. They make unit testing rather exciting and can do all sorts of surprising things.

Answer 1

Okay so first of the obligatory Singletons are bad practice so you probably shouldn't make it easy to write bad code.

Ignoring the fact that the class probably shouldn't exist at all we can look at the code.

static T& GetInstance() {
 static MemGuard g; // clean up on program end
 if (!m_instance) {
 m_instance = new T(); 
 }
 return *m_instance;
}

If multiple threads access this instance simultaneously before it is created, you have a data race and m_instance may end up be being constructed multiple times or other kinds of undefined behaviour. You need to add mutex locks around the if block or use std::call_once which is preferred.

As it is supposed to be a singleton you're not supposed to be able to create more instances as the meaning of a singleton is to just have one instance but it appears that it is fully possible to construct multiple instances of Test simply by creating them as local variables. So this is a design flaw in your template.

A much better way of creating a singleton is to rely on C++11 Magic Statics (N2660). And simply do this:

class Test{
private:
 Test(); // Disallow instantiation outside of the class.
public:
 Test(const Test&) = delete;
 Test& operator=(const Test &) = delete;
 Test(Test &&) = delete;
 Test & operator=(Test &&) = delete;
 static auto& instance(){
 static Test test;
 return test;
 }
}; 

Which is much easier to write than your code, it's thread safe and fixes the issues with allowing Test to be instantiated. The properties of magic statics guarantee that test will be initialised exactly once the first time the function body is entered by any thread, even in the presence of multiple threads that might otherwise cause a data-race. The instance will be deconstructed when your main() function returns (in the static destruction stage) which makes the whole MemGuard thing unnecessary.

• muXXmit2X: Okay, okay I know that Singletons often are nothing but over qualified global variables but there are a (very) few cases where they can be useful. I see your point in thread safety, which I've totally neglected. I will have a look at these magic statics. However you can't generate local variables of Test since its constructor is private in my implementation as well.
• Emily L.: Oh I misread the code, I saw TestIt() as a constructor. My bad. I guess that makes a case for naming :p
• shelper: just to note, that to get the instance of this class, you need to do:auto& instance = Test::instance()

Answer 2

Singletons make it hard to test your code, and in my job I'd reject this at review for encouraging the development of untestable features. That said, I'll continue reviewing despite that.

No need for helper class

The MemGuard appears to be a poor man's reimplementation of std::unique_ptr. It would be much simpler for you to declare m_instance as a std::unique_ptr<T>, and then just return *m_instance from your accessor.

There's a race condition when two or more threads try to create the instance (when both see a null pointer "before" the other has set it). You could work around this with a mutex lock, but it's simpler to use a local static variable, which is thread-safe:

#include <memory>
template<typename T>
T& Singleton<T>::instance()
{
 static const std::unique_ptr<T> instance{new T{}};
 return *instance;
}

---

We don't need a destructor

There's no need for the empty virtual destructor, as the constructed object will always be deleted as its declared type.

---

Revised implementation

With my changes, the code reduces to

template<typename T>
class Singleton {
public:
 static T& instance();
 Singleton(const Singleton&) = delete;
 Singleton& operator= (const Singleton) = delete;
protected:
 struct token {};
 Singleton() {}
};
#include <memory>
template<typename T>
T& Singleton<T>::instance()
{
 static const std::unique_ptr<T> instance{new T{token{}}};
 return *instance;
}

I'm using a constructor token to allow the base class to call the subclass's constructor without needing to be a friend.

Example

An example T looks like:

#include <iostream>
class Test final : public Singleton<Test>
{
public:
 Test(token) { std::cout << "constructed" << std::endl; }
 ~Test() { std::cout << "destructed" << std::endl; }
 void use() const { std::cout << "in use" << std::endl; };
};

Although the constructor is public, it can't be called without a Singleton<T>::token object, meaning that access to it is now controlled.

Tests:

int main()
{
 // Test cannot_create; /* ERROR */
 std::cout << "Entering main()" << std::endl;
 {
 auto const& t = Test::instance();
 t.use();
 }
 {
 auto const& t = Test::instance();
 t.use();
 }
 std::cout << "Leaving main()" << std::endl;
}

Entering main()
constructed
in use
in use
Leaving main()
destructed

---

Afterthought:

There's no need for the smart pointer; ordinary memory management works here:

template<typename T>
T& Singleton<T>::instance()
{
 static T instance{token{}};
 return instance;
}

• Emily L.: Blast from the past here... You're correct that you don't need the smart pointer, except if you're using MS VS13 which had a bug where it would hang on shutdown when deconstructing function scoped statics. The solution for us was to use a smart pointer... (Going off of memory here, we might actually have used a raw pointer and let it leak, don't actually remember if the static smart pointer had the same bug).

Answer 3

When working with static and shared libraries, one must be careful that you don't have several implementations of the instance() function. That would lead to hard to debug errors where there actually would exist more than one instance. To avoid this use an instance function inside a compilation unit (.cpp) and not in a template from a header file.