is there anything wrong with this Singleton class

Question

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 ?

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... — Sam Varshavchik, Oct 07 '16 at 17:46
Yeah there are a number of things wrong. You should actually use [Scott Meyer's singleton](http://stackoverflow.com/questions/17712001/how-is-meyers-implementation-of-a-singleton-actually-a-singleton) approach. — πάντα ῥεῖ, Oct 07 '16 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 !! — Dhia Hassen, Oct 07 '16 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. — user4581301, Oct 07 '16 at 17:54
As soon as you need to make a function like `Init` you have already lost. This won't be thread safe, you've exposed yourself to the possibility of `Init` not being called before the singleton is needed and the possibility multiple fools calling `Init` and possibly changing the rules of the simulation part way through even without multiple threads. — user4581301, Oct 07 '16 at 18:03
you don't know how does the game engine work , that `Init` function will be called from the engine's `Init'` function and it will be unfunctional before and after that call , and `GetInstance()` will throw exception if called before `Init()` , still asking , is there anyway to fix the multiple reference problem besides my implementation ? — Dhia Hassen, Oct 07 '16 at 18:08
@DhiaHassen _"is there anyway to fix the multiple reference problem besides my implementation ?"_ Yes. Change your implementation as you were advised to. — πάντα ῥεῖ, Oct 07 '16 at 18:16

Jarod42 · Accepted Answer · 2016-11-01T21:09:13.863

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;
    }
};

edited Nov 01 '16 at 21:09

answered Oct 07 '16 at 17:46

Jarod42

203,559
14
181
302

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 – Dhia Hassen Oct 07 '16 at 17:59
@DhiaHassen _"i want it to be inlined for performance"_ That would be inlined actually. – πάντα ῥεῖ Oct 07 '16 at 18:18
yeah it is now inlined , thanks i will use this but with using template – Dhia Hassen Oct 07 '16 at 20:52
`@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 http://stackoverflow.com/questions/22485432/singleton-pattern-performance-issue , so you are not sure about it – Dhia Hassen Oct 07 '16 at 21:02
2

[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! – Jason D Nov 01 '16 at 05:34

Guillaume Racicot · Answer 2 · 2016-10-07T19:59:50.097

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> { /* ... */ };

edited Oct 07 '16 at 19:59

answered Oct 07 '16 at 17:49

Guillaume Racicot

39,621
9
77
141

now how to call the GetInstance() of the derived class , cant do this : MySingleto::GetInstance() – Dhia Hassen Oct 07 '16 at 21:11
You should be able to do this: `MySingleton::instance()` – Guillaume Racicot Oct 07 '16 at 21:15
and this way too : Singleton::GetInstance() , both are the exact same method with the exact same reference – Dhia Hassen Oct 07 '16 at 21:18

score 1 · Answer 3 · answered Oct 07 '16 at 18:12

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.

answered Oct 07 '16 at 18:12

Evan Teran

87,561
32
179
238

yeah i was thinking about that too – Dhia Hassen Oct 07 '16 at 18:15
but that is plain c , i won't be able to pass an object of a typename to function when needed , still useful – Dhia Hassen Oct 07 '16 at 18:19
@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) – Evan Teran Oct 07 '16 at 19:18
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 – Dhia Hassen Oct 07 '16 at 20:46
1

@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. – Evan Teran Oct 08 '16 at 07:10

score -2 · Answer 4 · answered Oct 07 '16 at 17:48

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.

Perhaps you should clarify by providing your own answer instead — inetknght, Oct 07 '16 at 17:50
There is no point. Jarod42 typed it out faster than either of us. — user4581301, Oct 07 '16 at 17:51

is there anything wrong with this Singleton class

4 Answers4