RFC: singletons and memory management

[Soren Harward]

As a way of familiarizing myself with Amarok's internals, I've been
doing a light audit of its memory management to make sure there aren't
any severe leaks.  Amarok uses a lot of Singletons, and I've noticed
that very few of these ever get freed, and in the few cases that they
do, they are freed by way of "delete Singleton::instance()".  I've
always thought that this approach was Considered Harmful because it
leaves a dangling s_instance pointer within the class and will cause a
segfault in the event that something else calls Singleton::instance()
after it has been freed.  Right now, I haven't found any cases where
this happens, because the vast majority of the singleton classes in
Amarok never get freed in the first place.  But as of r859563, both
MainWindow.cpp and App.cpp are calling "delete
ScriptManager::instance()", which means there's at least one double
free in the code.  As we become more strict about memory management,
these problems are likely to be compounded.

So I have a suggestion/RFC for a general Amarok software design policy
as far as singleton classes are concerned.  A better practice for
singletons is to make the destructors private, and to provide a static
method called "destroy" (or something to that effect) that contains
the following:

void Singleton::destroy() {
  if (s_instance) {
    delete s_instance;
    s_instance = 0;
  }
}

Then the "Singleton::destroy()" function is called where the object
would otherwise be deleted.  I suggest that as a general Amarok
software development policy, that all singleton classes have private
destructors and a static destroy() method.  I can start making the
necessary changes in the existing singleton classes.  Comments?

-- 
Soren Harward