Steven Engelhardt

DevX.com has a number of RSS-backed tip of the day feeds, such as their C++ tip of the day. However, I often find myself shaking my head because the tip is either inane or terribly wrong. For example, take the tip A Better Way to Force a C++ Class to Be a Singleton:

class MySignleton
{
public:
    static MySingleton* GetInstance()
    {
       if(!m_pThis)
          m_pThis = new MySingleton();
       return m_pThis;
    }

    static void DeleteInstance()
    { if(m_pThis)
      {
       delete m_pThis;
       m_pThis = NULL;
      }
    }
private:
    MySingleton(){ m_pThis = NULL;}
    ~MySingleton();
    static MySingleton* m_pThis;
};

What’s wrong with this example, you ask? Well, first there’s the minor problem of the class name being misspelled. Furthermore, I question whether it is truly appropriate for a Singleton to have a DeleteInstance() method. Some may note that the class isn’t thread-safe. The implementor missed a golden opportunity to use templates. But worst of all is that the implementation is quite wrong and it won’t compile; even worse, if it did compile, it would likely crash. The problem? Doe has mishandled the declaration and initialization of the static m_pThis member of the class.

Static members of C++ classes are declared within the class, but the actual variable must be ‘allocated’ outside of the class. For example:

class MySingleton
{
    ...
};

MySingleton* MySingleton::m_pThis = NULL;

Note that in addition to ‘allocating’ m_pThis, you must also initialize it to NULL. If you do not perform the initialization, m_pThis will likely be a non-NULL random value and returned as-is by GetInstance(). A crash is virtually guaranteed upon its dereference. Of course, it is likely that this problem would only show up in a release build — there’s a good chance a debug build would set m_pThis to a value of NULL by default, in which case everything would appear fine.

Normally I would chalk this up to mere forgetfullness, but Doe has done something bizarre that makes me think he doesn’t realize what is happening. He sets m_pThis to NULL within MySingleton’s constructor! Consider what would happen:

1. Because of the line MySingleton* MySingleton::m_pThis = NULL;, the compiler allocates and initializes m_pThis to NULL before the program begins execution.
2. A user calls MySingleton::GetInstance().
3. m_pThis is NULL, which results in the line m_pThis = new MySingleton(); executing.
4. new MySingleton() calls MySingleton’s constructor, which sets m_pThis to NULL. (?!)
5. The new MySingleton() operation finishes and the result is assigned to m_pThis, setting it to the final, correct value.

As you can see, the constructor for MySingleton is absolutely the wrong place to set m_pThis to NULL, and it demonstrates Doe’s fundamental lack of understanding of what is happening. Incidentally, off the top of my head it seems a bad idea to change any static member from a non-static method, although I don’t discount the possibility of someone producing a valid counterexample.

I wish DevX.com performed better vetting of their tips.