A much better way of doing Singleton's (updated!)

[_tommo_]

Code: Select all

LogSingleton = Ogre::LogManager::getSingletonPtr();
LogSingleton->log("hello world");

That's quite a bit different

In the JohnJ code i have to call ONE time new, and then use everywhere LogSingleton... instead in ogre i have to use always both lines, or one with the usual ::getSingleton() that isn't exactly cool.

PS: i discovered now that you could avoid using getDefaultLog, thanks

[jacmoe]

You have a point, but if you just grab the singleton as LogSingleton, you can use LogSingleton->Log anywhere from within the class in which you grabbed it. So it doesn't make that much of a difference, really.
But I like JohnJ's version.

[_tommo_]

Yes in the classes where i use often singletons i do grab them in a pointer, but it is not so good, and Log for example is one of those classes that you need just one time but everywhere

Another "singleton flavour" i was using was a singleton manager: only the manager was singleton, and you registered "managers" to it... so that you could later retrieve them by type, like:

Singleton::get<WorldManager>()

But it's even longer, and in fact it is slower because has to do a search in the registered objects.

[nullsquared]

Or, you do it the way it's meant to be:

Code: Select all

std::clog << "logging this, bla bla";

[Klaim]

What about concurrent access with this singleton implementation?
From here it looks like using a Mutex or similar on the instance will not be effective...

Anyway, what's the point in allowing the 'new' operator at all?

[JohnJ]

What about concurrent access with this singleton implementation?
From here it looks like using a Mutex or similar on the instance will not be effective...

If I'm understanding you correctly, it would be pointless to lock a singleton from within the singleton accessor functions. Class-wide locks are bad and kill concurrency, because in this case you'd essentially be forcing other threads to wait on it to finish before continuing anyway. Concurrency is a problem that needs to be solved at a higher level (like making individual functions thread safe, designing communication patterns, etc.), and can't simply be fixed by locking entire classes.

Anyway, what's the point in allowing the 'new' operator at all?

Often you need to initialize a class with variables, pointers to structures, etc. - even singleton classes. This is just an intuitive way of letting your class initialize itself using C++'s "new" keyboard, rather than something like an "initialize" function. If you don't pass any parameters, there wouldn't be much point to allowing the new operator. In fact I could add a "StaticSmartSingleton" class for this, but for consistency it's probably best to stick with the current method.

[Klaim]

Often you need to initialize a class with variables, pointers to structures, etc. - even singleton classes. This is just an intuitive way of letting your class initialize itself using C++'s "new" keyboard, rather than something like an "initialize" function. If you don't pass any parameters, there wouldn't be much point to allowing the new operator. In fact I could add a "StaticSmartSingleton" class for this, but for consistency it's probably best to stick with the current method.

I see, I first thought you had to do this each time you wanted an access to the singleton object.
It would be good to separate singleton access instance construction from singleton unique instance construction I think.

I've seen something like that somewhere (maybe in a Boost singleton library proposal) :

Code: Select all

// Log have to be unique and provide global access - separately because the two concepts are not always wanted together
// GlobalAccess would define an type 'instance' that simply exposes the Log interface like in the Pimpl idiom or something...
class Log : public Unique<Log>, public GlobalAccess< Unique<Log>> { //... 
}
// or maybe something like Unique<Log, GlobalAccess> ? whatever, the implementation here is less important than the usage

// then...
// somewhere before usage, be it in global or function local or whatever
// we create and initialize the unique instance
Log theUniqueLog = new Log( /** blah blah blah **/ );

// somewhere where we need to access the log
void myFunction()
{
    Log::instance logger; // access object, exposes the unique instance interface, and is forbidden to be created by new or delete
    logger.log("hello world");

    // then the access object is automatically destroyed, but not the instance.
}

Or something like that. I'm sure this example is totally wrong but it's to get the global idea.
Would be interesting to see if something like that is really possible without too much drawbacks...I'll try an implementation when I get some time.

[JohnJ]

It would be good to separate singleton access instance construction from singleton unique instance construction I think.

You can already do that with my technique.

Singleton access instance construction:

Code: Select all

//Style 1
LogSingleton = new Log( /* blah blah blah */ );
//Style 2
Log = new Singleton::Log( /* blah blah blah */ );

Local instance construction:

Code: Select all

//Style 1
Log *log = new Log( /* blah blah blah */ );
//Style 2
Singleton::Log *log = new Singleton::Log( /* blah blah blah */ );

But I think if you need to do this it's not really singleton and you should consider finding a better solution.

[Klaim]

You don't seem to do the same thing I was talking about.

The accessor don't need to have construction parameters and don't need to be newed at all.

Anyway, some code would be more explicit.

[JohnJ]

Oh, I see. I thought you meant something completely different because I didn't look at the code example close enough. You can ignore my last post.

It would be good to separate singleton access instance construction from singleton unique instance construction I think.

To me, constructing an accessor instance is an additional level of complexity that doesn't really make sense with a singleton. A singleton is obviously a singular and global resource, and therefore you're not going to want to lock it under a mutex (that would kill concurrency), and storing a local pointer to a global object is ambiguous and error-prone.

If I ever found the need to access a singleton in any way but globally (like storing or passing around a local pointer of it), I wouldn't use a singleton at all. The only reason I've ever stored pointers to singleton classes in the past is because the ".getSingleton()" notation is so awkward and using a local pointer can smooth it over a little, at the expense of code bloat and unnecessary logic.

This is why I specifically didn't add a "getInstance()" method to the SmartSingleton helper class code I posted. You're free to add something like that yourself, but I wouldn't recommend it. I think it makes a lot of sense that something like this:

Code: Select all

void function1()
{
    Log->doThis();
    Log->doThat();
}

void function2()
{
    Log->doSomething();
}

Is a lot cleaner than this:

Code: Select all

void function1()
{
    Log::AcessorInstance myLog = Log->getInstance();
    myLog.doThis();
    myLog.doThat();
}

void function2()
{
    Log::AcessorInstance myLog = Log->getInstance();
    myLog.doSomething();
}

[Klaim]

Right, good points.

[JohnJ]

Update!
- Runtime checks added to throw an error if more than once instance of a singleton is ever created
- Compile-time check hack added to ensure that singletons aren't assigned from lvalues, which are misleading
- Added SMARTSINGLETON_STATIC mode which when defined automatically allocates your singletons in static memory (rather than heap), with no changes to your code required as long as your class has a default constructor.

Previously this wasn't a complete singleton technique as it did not perform runtime checks to detect an error if more than one instance of the singleton class is created. This version will automatically ensure that only one SmartSingleton is ever active, although it cannot know if somewhere in your code another instance is created like "Singleton::MyClass myClass = new Singleton::MyClass(...);"; for this a macro ("ENFORCE_SINGLETON_CONSTRUCTION;") can be added to your constructor to detect all possible singleton violations, although in most cases this really isn't necessary.

The new SMARTSINGLETON_STATIC mode allows you to almost automatically set your singletons to reside in static rather than heap memory, which is more efficient in most cases. The only change required is that your singleton class must implement a default constructor for the static allocation. You can still use other constructors in addition to that - calling "MyClass = new MyClass(...);" method will work as expected and still use static memory, although it might be preferred to call an initialize() method instead since using "new" in static mode requires that the SmartSingleton copy the data from the allocated memory to the static (this is done automatically of course, again no difference in usage code from the non-static version).

Another safety check is that now assigning an instance to an lvalue is not allowed:

Code: Select all

SmartSingleton<Singleton::MyClass> MySingleton;
...
Singleton::MyClass *myInstancePointer = new Singleton::MyClass(...);
MySingleton = myInstancePointer; //compile error

Code: Select all

SmartSingleton<Singleton::MyClass> MySingleton;
...
MySingleton = new Singleton::MyClass(...); //compiles and runs fine

Because this is confusing and error prone. For example if you tried to delete "myInstancePointer" yourself after assigning it to the SmartSingleton, your program would crash since ownership is transferred fully to the SmartSingleton when you do this and it expects to delete it automatically for you. Factory functions should still work fine.

Anyway, this was a fun little "template metaprogramming"-ish project, and will certainly make singletons a lot safer and easier for me. You can download the latest version from the first post of this thread.