Singleton Template questions - Code from Game Programming Gems

Started by
7 comments, last by GameDev.net 17 years, 3 months ago
Hi, I have some code from Game Programming Gems 1 and I have a few questions on it.

template <typename T> class Singleton
{
	static T * ms_Singleton;

public :
	Singleton(void)
	{
		assert(!ms_Singleton);

		int offset = (int)(T *)1 - (int)(Singleton <T> *)(T*)1;
		ms_Singleton = (T*)((int)this + offset);
	}

	~Singleton()
	{
		asset(ms_Singleton);
		ms_Singleton = nullptr;
	}

	static T & GetSingleton()
	{
		return (*ms_Singleton);
	}

	static T * GetSingletonPtr()
	{
		return (ms_Singleton);
	}
};

template <typename T> T* Singleton<T>::ms_Singleton = 0;

class Manager : public Singleton <Manager>
{
	public :
		void Print()
		{
			std::cout << "Manager Instance called" << std::endl;
		}
};

int main()
{
	Manager *m = Manager::GetSingletonPtr();
	m->Print();
}


The Manager pointer in main actually on debugging gives me a NULL pointer - 0x0000 but it still seems to work fine. How is this possible ? Secondly how does this code snippet exactly work. What exactly is happening here. int offset = (int)(T *)1 - (int)(Singleton <T> *)(T*)1; ms_Singleton = (T*)((int)this + offset); Thanks
The more applications I write, more I find out how less I know
Advertisement
Quote:The Manager pointer in main actually on debugging gives me a NULL pointer - 0x0000 but it still seems to work fine. How is this possible ? Secondly how does this code snippet exactly work. What exactly is happening here.


Make sure your not in release mode as that can cause symbols not to be updated but still show up in the debugger.
int offset = (int)(T *)1 - (int)(Singleton <T> *)(T*)1;ms_Singleton = (T*)((int)this + offset);
BLOWS UP THE UNIVERSE!!!!!!!!!!!!!!!!
But more on that soon.

What its intended to result in is offset = num bytes from base class pointer to derived class pointer and ms_Singleton = this as a derived pointer.

Now to expand on my earlier comment. Seriously those two line have so much undefined behaviour i dont know where to start. The two (T*)1's in the first line produces undefined behaviour you could avoid this with a reinterpret_cast<T*> but that results in the incorrect result (compared to what was intended) static_cast<T*> has the right semantics but results in the same undefined behaviour as (T*)1. Then casting the pointer to an int results in more undefined behaviour, reinterperat_cast<int>() could be used to avoid that bit of udefined behaviour but then any operation on the result except for reinterperate_cast<T*>() results in more undefined behaviour (in this case the subtraction) and finally you run in to the same problems trying to add the result to (int)this, which should be reinterpret_cast<int>(this) to avoid undefined behaviour, and casting that back to a T*.
int offset = (int)(T *)1 - (int)(Singleton <T> *)(T*)1;ms_Singleton = (T*)((int)this + offset);


Maybe a silly question, but why couldn't they just do:

ms_Singleton = static_cast<T*>(this);


Is use of static_cast() undefined when casting to "type*" when that object of that "type" is not yet fully constructed? IIRC using "this" as a pointer (or reference - "*this") just to pass the pointer around or store it somewhere is fully reliable (the standard points it out explicitly somewhere).
Quote:Is use of static_cast() undefined when casting to "type*" when that object of that "type" is not yet fully constructed? IIRC using "this" as a pointer (or reference - "*this") just to pass the pointer around or store it somewhere is fully reliable (the standard points it out explicitly somewhere).


IIRC static_cast<>() was poorly supported by compiler vendors when the book was written (same goes for the other *_cast<>()'s).
That code doesn't work, since it never allocates any objects - you have to do something like
int main(){	new Manager();	Manager *m = Manager::GetSingletonPtr();	m->Print();	delete Manager::GetSingletonPtr();}
else it'll crash when you try accessing any member variables of Manager, because (as you saw) m==NULL in your original code. The Print() method only works because it's not using 'this' at all.
Yeah. I've seen (unfortunately) many implementations of Singleton, and this one might be the worst. If the authors wanted to make a singleton that requires explicit instantiation (which they've done), then at a *minimum*, the two accessors (two? why? why!?!?!?) should ASSERT that ms_Singleton is not NULL.

But yeah, that constructor is just a pile of disasters waiting to happen.
Scott Bilas, the author of this particular Gem, has additional notes on his website regarding its implementation. He acknowledges the flaw in the constructor and recommends the static_cast instead.
Personally I prefer this version of the Singleton (in the rare event that I actually need one):

template<typename T>class Singleton{public:    static T& Get()    {        // This bit is the magic.  The standard guarenttees that static        // local variables will be initialized the first time that the        // function is called.  This means that order of construction is         // guarentteed and automagically occurs the first time Get() is        // called for the type T, regardless of where it's called from        // (eg. calling it from a global's or another singleton's        // constructor is perfectly safe).        static T instance;        return instance;    }private:    Singleton() {}};class MyClass{public:    void DoStuff() {}private:    // You only need this bit if you want to ensure    // the class is only used as a singleton    friend class Singleton<MyClass>;    MyClass() {}};void func(){    Singleton<MyClass>::Get().DoStuff();}
"Voilà! In view, a humble vaudevillian veteran, cast vicariously as both victim and villain by the vicissitudes of Fate. This visage, no mere veneer of vanity, is a vestige of the vox populi, now vacant, vanished. However, this valorous visitation of a bygone vexation stands vivified, and has vowed to vanquish these venal and virulent vermin vanguarding vice and vouchsafing the violently vicious and voracious violation of volition. The only verdict is vengeance; a vendetta held as a votive, not in vain, for the value and veracity of such shall one day vindicate the vigilant and the virtuous. Verily, this vichyssoise of verbiage veers most verbose, so let me simply add that it's my very good honor to meet you and you may call me V.".....V
Agreed with joanusdmentia. Singletons are the anti-OO design pattern and should be used when NOTHING else works. I've yet to come across a situation where a singleton wasn't just lazy programming. Logger? No need to be a singleton, you've over designed it most likely, a few handy global functions (in a namespace of course) would be much better. Consider the following as an example:

Log::Get().Write(..., ...);

vs

Log(..., ...);

I think using the second one is a lot cleaner. The global function can access a class with static members if needed.

Normally exposing a class to EVERYTHING should be a huge red flag of a really bad design going on. Time to refactor some code.

This topic is closed to new replies.

Advertisement