0

I am trying to implement a base class singleton using CRTP in a single threaded environnement where the instance is not held by the user, code is below:

#include <type_traits>
#include <atomic>
#include <cassert>

//Single threaded singleton.
//T is default constructable.
template<typename T>
class singleton
{
    static bool m_empty;

public:
    virtual ~singleton()
    {
        this->reset();
    }

    //YOU will end up with nullptr if I reset: YES THAT IS WHAT I WANT, YOU SHOULDN'T HAVE THE POINTER AT FIRST PLACE.
    //Calls should be done this way: singleton::instance()->do_stuff(); Never hold the instance ++ Single threaded.

    static T* instance()
    {
        static T* pme = new T();
        if (pme)
            return pme;
        m_empty = false;
        pme = new T();
        return pme;
    };

    static bool reset()
    {
        if(m_empty)
            return false;

        T* pme = instance();
        m_empty = true;
        delete pme;
        return true;
    };

protected:
    singleton() = default;
};

I can use the class as fellow:

class derive : public singleton<derive>
{
public:
    derive() = default;

    double get() const
    {
        return m_example;
    }
private:
    double m_example;
};

Q1. Is this ok please? (Please see the restriction above. I know this doesn't work in multithreaded environnement and if the instance is held).

Q2. I get error when doing this:

int main(int argc, char* argv[])
{
    const double d = derive::instance()->get();
};


>main.cpp
1>main.obj : error LNK2001: unresolved external symbol "private: static bool singleton<class derive>::m_empty" (?m_empty@?$singleton@0_NA)
1>..\build\bin\Debug\windows\x86_64\tests\tests.exe : fatal error LNK1120: 1 unresolved externals

Could you please help me?

Q3. I don't need this but is there any way to have a base using CRTP class singleton in a multithreaded env?

Vero
  • 313
  • 2
  • 9
  • 2
    What does "not held by the user" mean? Your linker error is because you have not provided a _definition_ (as opposed to a declaration) for `m_empty`. If you're using C++17 or later, you can declare it `inline`. – Paul Sanders Apr 25 '22 at 21:30
  • Also, a [mre] for this could be about 5 lines of code. – Paul Sanders Apr 25 '22 at 21:31
  • @Paul Sanders Works!! Thank You. Could you please explain what's heppening here. Not held by the user means the user can't do something like `derived* p = derived::instance()` and hold the instance because it could be deleted somewhere else. The only call as explained in my code is something like `derived::instance()->do_stuff()` in a single threaded env. Could you please give some background on the failure please? – Vero Apr 25 '22 at 21:38
  • https://en.cppreference.com/w/cpp/language/static – Paul Sanders Apr 25 '22 at 21:42
  • 1
    `public: derive() = default;` doesn't make `derive` much of a singleton. You may want to make the default constructor `private` and add `friend class singleton;` – Ted Lyngmo Apr 25 '22 at 21:48
  • 1
    @Ted Lyngmo right! Thanks I was thinking how to make it private.. Thanks all :) – Vero Apr 25 '22 at 21:52
  • _the instance ... could be deleted somewhere else_ Why? That seems like a bad idea to me. Does the singleton have state that changes or has to be completely replaced? (Because your example doesn't show that). I'm struggling to understand the motivation behind the design here. – Paul Sanders Apr 25 '22 at 22:09
  • @Paul Sanders Exactly what you said. The key word here is `reset`, I have to reset all the (inherting from) singletons after the user asks for a reset. I just made everything safe as I just have changed the code based on Ted Lyngmo (He didn't exactly say this but good hint) making everything in singelton private and an only one friend interface managing the (inherting from) singletons. Really the user can't even call `singleton::instance()` :) This sounds very safe to me as long as the interface which I own the implementation does not (I don't) mess things up. – Vero Apr 25 '22 at 22:18
  • 2
    Instead of keeping a static bool m_empty and having all of these problems you could set pme =nullptr upon reseting. Also, its more safe than keeping a deleted pointer (the above code would probably crash after reseting and calling instance, since on instance() you dont check for m_empty but for pme) – Angelos Gkaraleas Apr 25 '22 at 22:39
  • @aggelos garaleas Cool stuff cool stuff! Thanks. Actually as you can see from the headers I was trying to make this work in a multithreaded env using `std::atomic_bool m_empty` but changed mind at the end and kept the `m_empty`. Thanks for pointing that out! Will update! – Vero Apr 25 '22 at 22:43
  • @aggelos garaleas ah sorry I mean I agree with you to get rid of the `m_empty` but not with letting the already called `singleton::instance()` alive after reset, the goal is no one should hold the already called singletons. I have to free the memory and make sure no one is using the old instances! I am taking that risk and I want the program to crash in the contrary case (someone holding old instance). But as I said I own the implementation not the user (assuming I know what I do). – Vero Apr 25 '22 at 22:56
  • If you leave it like this and someones calls 'reset()', the next caller of 'instance()' will crash. 'If (pme)' will evaluate to true and a deleted pointer will be returned. In your comment you state that you will end up with nullptr when you reset, but that will not happen in the current code – Angelos Gkaraleas Apr 25 '22 at 23:14
  • @aggelos garaleas I don't get what you are saying, I mentioned the calls will be done as `singleton::instance()->do_stuff()` in a single threaded env and never `singleton* p = singleton::instance()`. There is no someone, I promise I will do it this way. Do you still think it will crash. Let me test it now :) . – Vero Apr 25 '22 at 23:22
  • 1
    @Vero for having a different singleton for each thread, you can have a look in QThreadStorage manual here in case Qt is an option https://doc.qt.io/qt-5/qthreadstorage.html – Angelos Gkaraleas Apr 25 '22 at 23:22
  • @vero the code of the answer will not crash (if you check for nullptr obviously). The code of the question will crash regardless if you call it via instance()->do_stuff(). Check to see – Angelos Gkaraleas Apr 25 '22 at 23:27
  • @aggelos garaleas gotcha! Thank you, the code in the question is missing `inline static bool m_empty = true`, but it might crash anyways. I beleive you, I will just go with the answer below haha. Thanks very much! – Vero Apr 25 '22 at 23:29

1 Answers1

0

singleton_interface is the interface managing the singletons and calls the instances (I own this class not the user). Final implementation:

class singleton_interface;

//Single threaded singleton.
//T is default constructable.
template<typename T>
class singleton
{   
    friend singleton_interface;
   
    static T* instance()
    {
        static T* pme = new T();
        if (pme)
            return pme;
        pme = new T();
        return pme;
    };

    static bool reset()
    {
        T* pme = instance();
        if(!pme)
            return false;

        delete pme;
        pme = nullptr;
        return true;
    };

protected:
    singleton() = default;

    virtual ~singleton()
    {
        this->reset();
    };
};

derived classes should do:

class derived: public singleton<derived>
{
    friend singleton<singleton<derived>>;
    //....
};

Thanks all! I will go with this.

Vero
  • 313
  • 2
  • 9
  • 1
    `static T* pme = new T();` can be just `static T* pme;` (as, being a `static`, it will be initially `nullptr`). That will save you a [threadsafe initialiser](https://stackoverflow.com/questions/8102125/is-local-static-variable-initialization-thread-safe-in-c11), which generates quite a bit of code behind the scenese and which,, being single-threaded, you don't need. Also `pme = nullptr;` isn't achieving anything, because your zeroing a _copy_ of the pointer. Future users of your singleton will therefore receive a stale point, and we all know what that means so you need to rethink that bit. – Paul Sanders Apr 26 '22 at 16:31
  • Personally, I would make `pme` a static member variable of the class. Then that problem goes away, and if you're using C++17 or later you can both declare it and define it in a single statement by declaring it `inline`, which is nice and convenient. But still only call `new` when someone calls `instance`. – Paul Sanders Apr 26 '22 at 16:33
  • Thanks a lot, for your first point: I want this "Future users of your singleton will therefore receive a stale point" :) otherwise they will use it so I want it to be null: I can add an `expired` flag and not delete however it's not simple to add + not deleting will consume memory if too many resets :) I agree with you it's not good and you wouldn'i want to see such thing in library but I need it, there is no work aroud "this" because "this" is a solution so people don't hold a stale pointer. Again I agree with you but no other solution. – Vero Apr 27 '22 at 21:21
  • Did you miss my second comment? Because that should fix it. – Paul Sanders Apr 27 '22 at 21:35