5

The idea is to have a Singleton in C++ deleted when the program ends. We learned this method of implementation in class:

class Singleton
{

private:
    static Singleton* the_singleton;

protected:
    Singleton()
    {
        static Keeper keeper(this);
        /*CONSTRUCTION CODE*/
    }
    virtual ~Singleton()
    {
        /*DESTRUCTION CODE*/
    }

public:
    class Keeper
    {

    private:
        Singleton* m_logger;

    public:
        Keeper(Singleton* logger):m_logger(logger){}

        ~Keeper()
        {
            delete m_logger;
        }
    };
    friend class Singleton::Keeper;

    static Singleton* GetInstance();
    {
        if (!the_singleton)
            the_singleton = new Singleton();
        return the_singleton;
    }
};

Singleton* Singleton::the_singleton = NULL;

The idea is that on the first time the Singleton is created, a static Keeper object will be created in the Singleton's C'tor, and once the program ends, that Keeper will be destroyed, and in turn will destroy the Singleton's instance it is pointing to.

Now, this method seems quite cumbersome to me, so I suggested to ditch the keeper class and make the Singleton's instance a static object of the getInstance method:

<!-- language: c++ -->

class Singleton
{

protected:
    Singleton()
    {
        /*CONSTRUCTION CODE*/
    }

    ~Singleton()
    {
        /*DESTRUCTION CODE*/
    }

public:
    static Singleton &getInstance()
    {
        static Singleton instance;
        return instance;
    }

    /*OBJECT FUNCTIONALITY*/
};

That way, the Singleton is constructed on the first call to the getInstance method, and destroyed once the program ends. No need for that keeper class.

I tested it and it worked just fine - the Singleton was created and destroyed at the right places. However, my TA said that this pattern is wrong, though he couldn't recall what exactly was the problem with it. So I'm hoping someone here has encountered this implementation before and can tell me what's wrong with it.

Waleed
  • 3,105
  • 2
  • 24
  • 31
Idan Arye
  • 12,402
  • 5
  • 49
  • 68
  • Its wrong but you don't know what the problem is? Thats... interesting. Also note that its probably not important considering how non-useful the singleton is in the first place. – alternative Jun 12 '11 at 00:45
  • 2
    Without even looking at the code, I can say that the TA's answer was indefensible. – Beta Jun 12 '11 at 00:47
  • 3
    @Someboddy: You have reinvented the Meyers singleton. – Billy ONeal Jun 12 '11 at 01:03
  • 3
    @Billy: It's probably also worth mentioning that Scott Meyers is a world expert on C++ and much more trustworthy than the TA. – Ben Voigt Jun 12 '11 at 01:13
  • Your TA is a fool. http://stackoverflow.com/questions/1008019/c-singleton-design-pattern/1008289#1008289 – Martin York Jun 12 '11 at 06:49
  • @someboddy: the most glaring mistake of your TA, is to teach the Singleton Anti-Pattern of course. One word of caution then: even though it appears neat and you'll probably want to test it out now that you now about it, remember that global state makes testing difficult and multi-threading nigh-impossible. My 2 cts... – Matthieu M. Jun 12 '11 at 10:02

3 Answers3

9

It's wrong, but your modifications didn't introduce the problems -- they've been there all along.

Right now there's no enforcement of the singleton property. The constructor needs to be private, not protected, in order to ensure additional instances aren't created.


In addition to that, the original code is completely unusable in a multithreaded scenario. Yours will work beginning with C++0x.


If the TA's keeper object had reset the global pointer back to NULL, his implementation would be able (in a single threaded environment) to recreate the singleton if needed during program cleanup. Read Singleton pattern in C++ . But this still would be a new instance of the singleton, which is almost as unexpected as using the singleton after its destructor has been called.

However, he neglected to do that, so his will happily use an invalid pointer during program shutdown after the keeper deletes the singleton. In yours, at least the memory region continues to be valid even if the object's lifetime has ended.


You will of course be marked down for using the improved version, this simply shows that the aim of the class is not to teach you C++, but the TA's misconceptions about C++.

Community
  • 1
  • 1
Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • While this implementation does not enforce the singleton property, it is wrong to say that the constructor needs to be private (or even protected) in order to ensure additional instances are not created. – David Hammen Jun 12 '11 at 01:23
  • 1
    @David: What alternate enforcement mechanism do you have in mind? – Ben Voigt Jun 12 '11 at 01:26
6

There's no problem with it at all. The "Keeper" thing is nuts for this application, IMHO.

There are a few cases where it might be warranted. For instance, if the Singleton must take constructor arguments, allocating it statically might not be possible. Or if its constructor might fail, the simpler approach wouldn't allow you to recover or retry. So in some circumstances, doing something more complex might be needed. In many cases, though, it's unnecessary.

Ernest Friedman-Hill
  • 80,601
  • 10
  • 150
  • 186
  • 1
    +1, but could you elaborate? How can it be possible to allocate the instance the first way, but impossible the second? The two methods look almost identical. – Beta Jun 12 '11 at 01:01
  • I can see the point about construction failure. But arguments should not be a hindrance to use of a static local, after all, the TA's version passes arguments to the static local keeper object. – Ben Voigt Jun 12 '11 at 01:34
  • On MinGW, you can initialize a static variable with the method's arguments, and I see no reason why is should be different in other compilers. The constructor failure reason is good enough reason to keep that implementation in mind, though. – Idan Arye Jun 12 '11 at 01:56
  • I guess "must take constructor arguments" isn't really what I was thinking of here; more along the lines of "must be built up from several method calls using those arguments." If the Singleton object needed to be constructed and then you needed to call a separate `init(xyz)` method on it, for instance; this would be decidedly trickier to do with the static version. Basically any time the construction process is more than one line of code -- a `try` block, a series of method calls -- the dynamic version is going to be easier to manage. – Ernest Friedman-Hill Jun 12 '11 at 02:07
0

"In yours, at least the memory region continues to be valid even if the object's lifetime has ended"

This is true, but doesn't have to mean much for the program execution - destructor might leave memory region in state in which using it will fail anyway - eg. freeing internal buffer. You should NEVER use object after it's destructor has been called - if you want to use your singleton from destructors of other static/global objects, then you don't have any guarantee on order in which those destructors will be called.

If you use static object, and some other destructor will try to use it after it's been destroyed, there will be no way to check if singleton still exists. If you use keeper object that will set pointer to NULL then in other destructors you can at least check for NULL pointer, and decide not to use singleton at all. This way, although you might fail at what you intend to do, at least you will not get cryptic "segmentaion fault" or "access violation", and the program will avoid abnormal termination.

EDIT: You need to remeber to modify your accessor function not to create object once again after your keeper will set this field to NULL - perhaps additional static bool that states if object was alread freed?

j_kubik
  • 6,062
  • 1
  • 23
  • 42
  • It's easy to solve the order of instantiation problem for both construction and destruction. http://stackoverflow.com/questions/335369/finding-c-static-initialization-order-problems/335746#335746 – Martin York Jun 12 '11 at 06:53
  • That can easily be solved with a static boolean variable that indicates if we have an instance of the Singleton or not. – Idan Arye Jun 12 '11 at 10:16