C++ checking singleton pointers

Question

Hello,

I have an application, which has a (Qt C++) singleton logger class. The GetInstance() implementation is:

if(m_Instance == NULL)
{
    try
    {
        m_Instance = new Logger();
    }
    catch(...){}
}
return m_Instance;

Now I have following macro in the .h file: "#define LOG Logger::Instance()->Log"

Everything is fine as long as new() operation works. What is the best way to ensure that pointer has set (I was thinking some try-catch blocks to catch the std::bad_alloc, but I don't know how to implement that in a macro)? I have created a workaround, which seems to work but is not pretty:

"#define LOG if(Logger::Instance()) Logger::Instance()->Log"

In addition, I would like to know what if my object has a lot getters/setters (e.g. setPath(), getSize()...)? Currently, I have a macro:

"#define SET Settings::Instance()"

and in code I'm able to use SET->setPath("abc"); or SET->getSize();

In this case my ugly workaround does not work, because it requires separate macro for every method. Any tips how I can improve that?

Thanks for the answers.

Answer 1

You have already caught the exception in your GetInstance method, so nothing would ever emerge into the macro anyway. Perhaps...

if(m_Instance == NULL)
{
    try
    {
        m_Instance = new Logger();
    }
    catch (std::bad_alloc &ba)
    {
        // do something here...
    }
    catch(...){}
}

return m_Instance;

Answer 2

Don't use a terrible design pattern -> don't get problems. Normally, people use something more akin to

Logger& Logger::GetInstance() {
    static Logger instance; 
    return instance;
}

But the singleton pattern is in general absolutely terrible. Use the application instance pattern instead.

Answer 3

You don't say what you want to do if you haven't got a log.

If you want the error to propagate up, either don't catch the exception or throw more specific one to indicate a lack of log.

If you want to carry on regardless, either have a null check on every use, or use the null object pattern to replace the procedural check with a virtual method.

Answer 4

First of all, do you really want your application to silently ignore everything you are doing with these singletons should they fail to be allocated? For something like a logger, you might want to use it to report exception messages, so perhaps you don't want to allow it to throw as well.

For such cases, consider calling the instance method when your application starts (ex: in your main entry point) up so that if the application starts up successfully, it will always have a logger handy, e.g.

Finally, I recommend that m_instance uses something like boost::scoped_ptr so that you aren't leaking memory (or just store the logger as a static object in the instance method without the pointer indirection). Sure, modern operating systems will generally clean up after you, but if you start doing memory analysis to check for leaks, you're going to get a lot of false positives this way.

As for these macros, I think they are atrocious, but that's just my opinion. You could achieve similar effects without macros:

void write_to_log(Logger* logger, const char* msg)
{
    if (logger)
        logger->log(msg);
}

or even:

void write_to_log(const char* msg)
{
    Logger* logger = Logger::instance();
    if (logger)
        logger->log(msg);
}

Answer 5

If you are sure about singleton pattern (which seem to be overused here), then:

struct ILogger {
    void Log(const char *message) = 0;
};

struct NullLogger : ILogger {
    void Log(const char *message) { }
};

struct Logger : ILogger {
private:
    static ILogger *m_instance;
    static const NullLogger m_null_instance;

public:
    void Log(const char *message) { /* ... */ }

    static ILogger *Instance() {
        if(m_Instance != &m_null_instance) return m_Instance;

        try { m_Instance = new Logger(); } catch(...) { }

        return m_Instance;
    }
};

const NullLogger Logger::m_null_instance;
ILogger *Logger::m_instance = &Logger::m_null_instance;

/* ... */
#define LOG Logger::Instance()->Log
LOG("Hey!");

Answer 6

(1) As others mentioned: Don't use singletons. (2) If you use a singleton, use one with a solid implementation. (3) If you want a HACK:

void Logger::Log( /*params*/ )
{
  if( NULL != this )
  {
    // do your logging
    ...
  }
}

Answer 7

Thanks for the replies.

My singleton implementation is created based on various examples available in the web. ( e.g. http://www.yolinux.com/TUTORIALS/C++Singleton.html is pretty close to mine).

The reason why I'm using singleton is to ensure that there is only one instance of the class (logger / settings) and yes I know it is overused.

However, it looks like I have to go and try to implement something similar without using the singleton.