0

I have created a Singleton class and I am wondering if my destructor functon will automatically release memory for the static variable named instance.

Will the following code automatically release memory?

class SingletonClass
{
    SingletonClass() 
    {

    }

    ~SingletonClass()
    {
       delete this; // or should I say... delete instance;
    }

    public:

    static SingletonClass* instance;

    SingletonClass* getInstance()
    {
       if (instance != NULL)
          return instance;

       instance = new SingletonClass();
       return instance;
    }
};

PS: Is it possible to just make instance a regular Singleton variable as opposed to a pointer? Would it be better code practice?

sazr
  • 24,984
  • 66
  • 194
  • 362

2 Answers2

4

This singleton class merrily injects Undefined Behavior in your program.

You have a static member variable which has automatic storage. As a global variable, it will get constructed before your main() routine is entered, and destructed after your main() routine exits.

Therefore, once your program terminates and exits the main() function, the destructor of your SingletonClass instance will be invoked, and it will try to delete this; however, the object was not allocated through a call to new, and calling delete for an object that was not allocated through new gives Undefined Behavior.

You can safely remove the delete this instruction: global objects are destroyed automatically when your program terminates.

EDIT:

After the edit to your question, what used to be a static variable of type SingletonClass became a static variable of type SingletonClass*. I suggest you changing it back:

static SingletonClass instance;

SingletonClass* getInstance()
{
    return &instance;
}

Actually, instance could (and probably should) even be a static local variable of function getInstance():

SingletonClass* getInstance()
{
    static SingletonClass instance;
    return &instance;
}

This way you would not even have to provide a global definition for a static class member variable.

Alternatively, you can use smart pointers for taking care of the object's lifetime, but that's unnecessary here. Just declare the variable as a static local variable of getInstance(). In C++11, its initialization would be also guaranteed to be thread-safe.

Andy Prowl
  • 124,023
  • 23
  • 387
  • 451
  • if I remove `delete this;` from the function, wont I have memory I have requested and never given back? – sazr Feb 21 '13 at 02:26
  • @JakeM: No. That's only the case when you obtain memory through a call to `new`. Here you have a global variable which gets automatically destroyed when your program terminates. The short story is: you haven't allocated it through `new`, so you shouldn't deallocate it through `delete`. – Andy Prowl Feb 21 '13 at 02:29
  • @JakeM: Oh wait: the global variable is a pointer? Then I must edit my answer. – Andy Prowl Feb 21 '13 at 02:30
  • instance is of the type `static Singleton*` not `static Singleton`, that was a typo. So afaik its allocated by a call to new within the function `getInstance()` – sazr Feb 21 '13 at 02:31
  • @JakeM: OK, then in that case it should have been `delete`d, yes, but not inside the class's destructor (the destructor is called *after* `delete`), so someone must have called `delete` already if you are in the destructor. At that point, calling `delete this` would deallocated the object twice. – Andy Prowl Feb 21 '13 at 02:35
  • ok so the destructor doesn't automatically get called after main returns? – sazr Feb 21 '13 at 02:37
  • @JakeM: If you have a raw pointer, no. If you have a smart pointer or a `static` variable with automatic storage (so: `SingletonClass`, not `SingletonClass*`), then it will be called automatically. – Andy Prowl Feb 21 '13 at 02:39
3

Use smart pointer like

 static std::unique_ptr<SingletonClass> instance;

It deletes the instance for you when program terminates. Avoid delete this in members of your code unless you really know what you are doing and what are consequences.

Öö Tiib
  • 10,809
  • 25
  • 44