-2

Under these condition i wrote the next Singleton class :
1 - i want one and only one instance of the class to be present and to be accessible from the whole game engine .
2 - the Singleton is intensively used ( thousands times per frame) so i dont want to write an extra GetInstance() function , im trying to avoid any extra function call for performance
3 - one possibility is to let the GetInstance() be inlined like this : 

inline Singleton* Singleton::GetInstance() { static Singleton * singleton = new Singleton(); return singleton; }

but that will cause a reference problem , on each call there will be a new reference to the singleton , to fix that wrote in c++ :

class Singleton{ private: static Singleton* singleton; Singleton(){} public: static inline Singleton* GetInstance() // now can be inlined ! { return singleton; } static void Init() { // ofc i have to check first if this function // is active only once if(singleton != nullptr) { delete singleton; } singleton = new Singleton(); } ~Singleton(){} // not virtual because this class can't be inherited }; Singleton* Singleton::singleton = nullptr;

What are the possible problems i can face with this implementation ?

Improve this question
8
  • No, on each call you will not get a new reference, in your initial, simple implementation. On the other hand, your over-engineered version does silly things like delete-ing nullptrs in an Init() function that does not appear to be called from anywhere... Commented Oct 7, 2016 at 17:46
  • 2
    Yeah there are a number of things wrong. You should actually use Scott Meyer's singleton approach. Commented Oct 7, 2016 at 17:48
  • thanks for noting , i fixed the Init() function , it is acctualy != and not== null , and when i inlined the first version i got a multiple reference error !! Commented Oct 7, 2016 at 17:50
  • By the way, inline is effectively useless with a modern compiler. The compiler will inline what it sees an advantage to inlining and totally ignore your request if the result would be, in its estimation, a bad idea. Commented Oct 7, 2016 at 17:54
  • @DhiaHassen "thanks for noting ..." De nada :P ... Commented Oct 7, 2016 at 17:55

4 Answers 4

Reset to default
11

Your first implementation problem is a leak of the only new you call. And the signature that force user to check pointer validity.

Your second implementation has even more problem as you require to use a 2-step initialization, and don't forbid copy/move/assignment.

Simply use Meyers' singleton:

class Singleton{ private: Singleton() = default; ~Singleton() = default; Singleton(const Singleton&) = delete; Singleton operator&(const Singleton&) = delete; public: static Singleton& GetInstance() { static Singleton instance; return instance; } };
Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

i don't want to call GetInstance() , i dont want the engine to call it 1000 time per frame , i want it to be inlined for performance , but when i inline it there will be multiple referenced of "static Singleton instance" on each call a new reference , that is why i wrote that implementation
@DhiaHassen "i want it to be inlined for performance" That would be inlined actually.
yeah it is now inlined , thanks i will use this but with using template
@remudada can this function be safely inlined ? Should be possible. – πάντα ῥεῖ Mar 18 14 at 16:56 , that was your comment on the accepted answer for this question stackoverflow.com/questions/22485432/… , so you are not sure about it
[flagged] This doesn't actually answer the OP's question of what's could be anything wrong with his code. Please explain what would lead you to use this code over what the OP wrote. "It's better" is not a sufficient answer. Please provide WHY it's better. What does it do that his does not...etc. Thanks!
5

In addition to @Jarod42's answer, I would like to point out that you could also implement a generic singleton by making template and use it in a CRTP class:

template<typename T> class Singleton { protected: Singleton() = default; ~Singleton() = default; Singleton(const Singleton&) = delete; Singleton operator&(const Singleton&) = delete; public: static T& instance() { static T instance; return instance; } };

Then extend it:

struct MySingleton : Singleton<MySingleton> { /* ... */ };
Improve this answer

3 Comments

now how to call the GetInstance() of the derived class , cant do this : MySingleto::GetInstance()
You should be able to do this: MySingleton::instance()
and this way too : Singleton<MySignleton>::GetInstance() , both are the exact same method with the exact same reference
1

Instead of a singleton, consider a namespace! Here's how I would do it:

// thing.h namespace thing { // public interface int doSomething(); } 
// thing.cpp namespace thing { namespace { // private data and functions can go right here :-) int private_data_ = 1234; int doSomethingInternal() { return private_data_ * 2; } } // public interface int doSomething() { return doSomethingInternal(); } }

Usage is simple like this:

int x = thing::doSomething();

No need for getInstance(), no memory leaks, and you can't accidentally make multiple instances.

Improve this answer

5 Comments

yeah i was thinking about that too
but that is plain c , i won't be able to pass an object of a typename to function when needed , still useful
@DhiaHassen, I think you are confused. It is very much not plain C, C doesn't have namespaces. If you are talking about the ability to "pass an object of type thing to a function".. Why would need to? There is only one of it by definition, you can just have that function use thing directly. (Which it could in the singleton case anyway)
i ment it does't define a new type , i know what C is , i ment that your interface here is C based , i was talking about the interface ( no classes ) , again still useful
@DhiaHassen, OK, well my question is. If you want a singleton, why do you need it to define a type? What is the utility of it? What does it being an object give you that a namespace doesn't in your use case? I can imagine some cases, but honestly I find that the vast majority of the time, a namespace covers all of the needs of a singleton, but without the downsides.
-2

but that will cause a reference problem , on each call there will be a new reference to the singleton

Incorrect; instead there will be a new class instance which is not the same as a reference. You will most likely end up leaking memory.

static Singleton* singleton;

Use a unique_ptr instead of a raw pointer. Compiler optimizations will devolve it into a raw pointer anyway, but now you're clearly telling the compiler what its lifespan should be.

class Singleton{ private : static Singleton* singleton;

The default scope of a class is private; you don't need to explicity say private scope.

Singleton(){}

There is no need to provide an empty constructor when you have no other constructors in the class.

im trying to avoid any extra function call for performance

Compiled C++ will often inline such code anyway.

 inline Singleton* GetInstance() // now can be inlined !

Make it static...?

 ~Singleton(){} // not virtual because this class can't be inherited

If your intent is to make it not inheritable, then add a final keyword to the class declaration. You can then remove the destructor.

Improve this answer

5 Comments

That's not idiomatic.
What's not idiomatic?
Everything you wrote.
Perhaps you should clarify by providing your own answer instead
There is no point. Jarod42 typed it out faster than either of us.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.