Don't use the Singleton pattern
But do you think it is really thread safe good singleton?
Please excuse my pedantry and let me say that the only good use of the Singleton pattern is to not use it at all. This is not to say that a program should never have just a single instance of a class. On the contrary, it is a perfectly reasonable thing at times. Just don't couple the “unique object” property to the type itself as the pattern does. Instead, write a normal class with a public constructor and let another class provide a unique instance of it. The internet is full of rants about the Singleton pattern and good reasons why not to use it so I'll leave it at that.
Watch your #include
s
If you're using the typeid
operator, you must #include
the <typeinfo>
header first.
The #include "stdafx.h"
, on the contrary, is non-portable and annoying. Get rid of it.
For std::runtime_error
, you don't need the <exception>
but the <stdexcept>
header.
Use vertical and horizontal white-space for better readability
It might be just an artifact of copying and pasting your code from your editor into the web browser but as it stands, your code is not very well formatted.
I would put a blank line between functions and classes as well as after the #include
directives at the top. The only blank line in your code is – in an empty function body.
There are many opinions about the optimal amount of horizontal space you use to indent your code and I'm okay with most styles. What is not okay is inconsistency. Pick one style and stick to it. Some lines of your code appear completely off. For example, the closing brace of Singleton::getinstance
(looks as if it were the end of the class) or the try
in main
.
Any decent editor can re-indent the code for you at the press of a button.
What's this std::atomic_flag
?
I'm not sure what problem exactly the Singleton::flag
member is supposed to solve but I'm pretty sure that there's a better way. If you don't want client code to instantiate your singleton class directly (which, as mentioned, is a poor decision, but ignore that for now) just make the constructor private
. Then the compiler can enforce at compile-time that nobody except the singleton provider instantiates it. This is much better than checking at run-time and crashing the program.
Reconsider the heap allocation
I don't see why you have to allocate your singleton instance dynamically. The only situation where I could imagine a problem with static deinitialization is if you refer to the object from within a function registered as an std::atexit
handler. On the contrary, dynamically allocating the object and then “leaking” it might cause leak detectors such as Valgrind to give you “false” positives that obscure more severe issues with your code. I prefer code to always clean up everything such that it is Valgrind clean. If you're writing a library, it is even more frustrating for your users to get leaks reported that they cannot do anything about.
Returning a reference is fine
I see many Singleton implementations for C++ return
ing a pointer just because they can. I like your choice of using a reference better.
You don't need an empty destructor
Your inline definition of a non-virtual
empty destructor adds nothing. It might only confuse people whether you might have actually forgotten to put a delete
of the instance there. (In which case it would of course have been better to use a std::unique_ptr
and again write no destructor.)
Make delete
d member functions public
It might sound counter-intuitive at first but making delete
d members functions public
gives you better error messages. It also makes sense if you think about it: The fact that your type is not copyable is part of its public interface.
Did you mean MyClass
to inherit private
ly from Singleton<MyClass>
?
I think it would be less surprising if you'd inherit public
ly because then you could also say MyClass::getinstance()
instead of the more verbose and less intuitive Singleton<MyClass>::getinstance()
.
The code in MyClass
is completely redundant
If you delete everything inside the MyClass
body, your program does the exact same thing. I'm sure that you did intend something by writing it but made some mistake (such as accidentally making the constructor public
) that renders it useless as it stands.
Get rid of the getchar
abomination in main
Don't make your program block on an I/O operation just because you want the terminal emulator window stay open. Instead, learn how to use the emulator such that the window stays open even after the program finished.
If you really want it, it should be std::getchar
and you need the <cstdio>
header.
How I would do it
Putting the previous advice together, I would solve the problem like this.
template <typename T>
struct singleton_provider
{
static T&
instance()
{
static T theinstance {};
return theinstance;
}
};
struct my_class
{
my_class(const my_class&) = delete;
my_class& operator=(const my_class&) = delete;
};
using my_class_provider = singleton_provider<my_class>;
It is a lot simpler, more efficient, doesn't leak memory and – most of all – doesn't couple the logic how objects of a class are instantiated and their lifetime is managed in a program to its type.
std::atomic_flag
, please? \$\endgroup\$