Qt C++: Global objects vs. reference chain

Question

Hello,

Currently I'm using singleton pattern for certain global objects in my application (Qt application for Symbian environment). However, because of some problems (http://stackoverflow.com/questions/3147375/c-checking-singleton-pointers/3147806#3147806) it looks like that I have to change the logic.

I have 3 classes (logger, settings and container for some temp data) that I need to access via multiple different objects. Currently they are all created using singleton pattern. The logger basically is just one public method Log() with some internal logic when the settings and container have multiple get/set methods with some additional logic (e.g. QFileSystemWatcher). In addition, logger and settings have some cross-reference (e.g. logger needs some settings and settings logs errors).

Currently everything is "working fine", but there is still some problems that should be taken care of and it seems like they are not easy to implement for the singletons (possible memory leaks/null pointers). Now I have two different ways to handle this:

1. Create global objects (e.g. extern Logger log;) and initialize them on application startup.
2. Create objects in my main object and pass them to the children as a reference.

How I have few questions related to these:

Case 1.

• Is it better to use stack or heap?
• I'm going to declare those objects in some globals.h header using extern keyword. Is it ok?
• I think in this case I have to remove that 2-way reference (settings needs logger and vice versa.)?

Case 2.

• Should the objects be created in stack or heap in my main object (e.g. Logger *log = new Logger() vs Logger log;)

• Long reference chains do not look nice (e.g. if i have to pass the object over multiple childrens).

• What about in children?

  1. If I pass a pointer to the children like this (I don't want to copy it, just use the "reference"): Children(Logger *log) : m_Log(log) what happens when the children is deleted? Should I set the local pointer m_Log to NULL or?
  2. If I use stack I'll send reference to the child (Children(Logger &log) : m_Log(log)) where m_Log is a reference variable (Logger& m_Log;) right?

• What should I note in terms of Qt memory management in this case?

Case 3. Continue with singleton and initialize singleton objects during the startup (that would solve the null pointers). Then the only problem would possible memory leaks. My implementation follows this example. Is there a possible memory leak when I'm accessing the class using. What about singleton destruction? #define LOG Logger::Instance()->Log

Thanks for reading.

Answer 1

Summary in simple terms:

1. if you use global objects, prefer the singleton pattern as a lesser evil. Note that a singleton should have global access! Dan-O's solution is not really a singleton pattern and it defeats the power of singletons even though he suggests it's no different.
2. if you use global objects, use lazy construction to avoid initialization order problems (initialize them when they are first accessed).
3. if you use singletons, instead of making everything that needs to be globally acccessible a singleton, consider making one singleton (Application) which stores the other globally-accessible objects (Logger, Settings, etc.) but don't make these objects singletons.
4. if you use locals, consider #3 anyway to avoid having to pass so many things around your system.

[Edit] I made a mistake and misplaced the static in safe_static which Dan pointed out. Thanks to him for that. I was blind for a moment and didn't realize the mistake based on the questions he was asking which lead to a most awkward situation. I tried to explain the lazy construction (aka lazy loading) behavior of singletons and he did not follow that I made a mistake and I still didn't realize I made one until the next day. I'm not interested in argument, only providing the best advice, but I must suggest strongly against some of the advice, particularly this case:

#include "log.h"

// declare your logger class here in the cpp file:
class Logger
{
// ... your impl as a singleton
}

void Log( const char* data )
{
    Logger.getInstance().DoRealLog( data );
}

If you are going to go with globally accessible objects like singletons, then at least avoid this! It may have appealing syntax for the client, but it goes against a lot of the issues that singletons try to mitigate. You want a publicly accessible singleton instance and if you create a Log function like this, you want to pass your singleton instance to it. There are many reasons for this, but here is just one scenario: you might want to create separate Logger singletons with a common interface (error logger vs. warning logger vs. user message logger, e.g.). This method does not allow the client to choose and make use of a common logging interface. It also forces the singleton instance to be retrieved each time you log something, which makes it so that if you ever decide to steer away from singletons, there will be that much more code to rewrite.

Create global objects (e.g. extern Logger log;) and initialize them on application startup.

Try to avoid this at all costs for user-defined types, at least. Giving the object external linkage means that your logger will be constructed prior to the main entry point, and if it depends on any other global data like it, there's no guarantee about initialization order (your Logger could be accessing uninitialized objects).

Instead, consider this approach where access is initialization:

Logger& safe_static()
{
    static Logger logger;
    return logger;
}

Or in your case:

// Logger::instance is a static method
Logger& Logger::instance()
{
    static Logger logger;
    return logger;
}

In this function, the logger will not be created until the safe_static method is called. If you apply this to all similar data, you don't have to worry about initialization order since initialization order will follow the access pattern.

Note that despite its name, it isn't uber safe. This is still prone to thread-related problems if two threads concurrently call safe_static for the first time at the same time. One way to avoid this is to call these methods at the beginning of your application so that the data is guaranteed to be initialized post startup.

Create objects in my main object and pass them to the children as a reference.

It might become cumbersome and increase code size significantly to pass multiple objects around this way. Consider consolidating those objects into a single aggregate which has all the contextual data necessary.

Is it better to use stack or heap?

From a general standpoint, if your data is small and can fit comfortably in the stack, the stack is generally preferable. Stack allocation/deallocation is super fast (just incrementing/decrementing a stack register) and doesn't have any problems with thread contention.

However, since you are asking this specifically with respect to global objects, the stack doesn't make much sense. Perhaps you're asking whether you should use the heap or the data segment. The latter is fine for many cases and doesn't suffer from memory leak problems.

I'm going to declare those objects in some globals.h header using extern keyword. Is it ok?

No. @see safe_static above.

I think in this case I have to remove that 2-way reference (settings needs logger and vice versa.)?

It's always good to try to eliminate circular dependencies from your code, but if you can't, @see safe_static.

If I pass a pointer to the children like this (I don't want to copy it, just use the "reference"): Children(Logger *log) : m_Log(log) what happens when the children is deleted? Should I set the local pointer m_Log to NULL or?

There's no need to do this. I'm assuming the memory management for the logger is not dealt with in the child. If you want a more robust solution, you can use boost::shared_ptr and reference counting to manage the logger's lifetime.

If I use stack I'll send reference to the child (Children(Logger &log) : m_Log(log)) where m_Log is a reference variable (Logger& m_Log;) right?

You can pass by reference regardless of whether you use the stack or heap. However, storing pointers as members over references has the benefit that the compiler can generate a meaningful assignment operator (if applicable) in cases where it's desired but you don't need to explicitly define one yourself.

Case 3. Continue with singleton and initialize singleton objects during the startup (that would solve the null pointers). Then the only problem would possible memory leaks. My implementation follows this example. Is there a possible memory leak when I'm accessing the class using. What about singleton destruction?

Use boost::scoped_ptr or just store your classes as static objects inside an accessor function, like in safe_static above.

Answer 2

I found the other answer to be a little misleading. Here is mine, hopefully the SO community will determine which answer is better:

Is it better to use the stack or the heap?

Assuming by "the stack" you meant as a global (and thus in the data segment), don't worry about it, do whichever is easier for you. Remember that if you allocate it on the heap you do have to call delete.

I'm going to declare those objects in some globals.h header using extern keyword. Is it ok?

Why do you need to do that? The only classes that need access to the globals are the singletons themselves. The globals can even be static local variables, ala:

class c_Foo
{
    static c_Foo& Instance()
    {
        static c_Foo g_foo; // static local variable will live for full life of the program, but cannot be accessed elsewhere, forcing others to use cFoo::Instance()
        return g_foo;
    }
 };

If you don't want to use the static local then a private static member variable of type c_Foo (in my example) would be more appropriate than a straight global.

Remember, you want the lifetime of the class to be "global" (ie not destroyed until application exit), not the instance itself.

I think in this case I have to remove that 2-way reference (settings needs logger and vice versa.)?

All of the externed globals would have to be forward declared, but as I said above you don't need this header file.

Should the objects be created in stack or heap in my main object (e.g. Logger *log = new Logger() vs Logger log;)

I can't really answer this, don't worry about it again.

Long reference chains do not look nice (e.g. if i have to pass the object over multiple childrens).

What about in children?

Great points, you have realized that this would be a huge pain in the butt. In the case of something like a logger it would be hellish to pass references to every module just so they could spit out logging info. More appropriate would be a single static "Log" function ala C, if your logger has useful state then make it a singleton that is only visible to your log function. You can declare and implement your entire logger class in the same .cpp file that you implement your log function in. Then nothing else will be aware of this special functionality.

Here is what I mean:

file: log.h

#ifndef LOG_H
#define LOG_H

void Log( const char* data ); // or QString or whatever you're passing to your logger

#endif//LOG_H

file: log.cpp

#include "log.h"

// declare your logger class here in the cpp file:
class Logger
{
// ... your impl as a singleton
}

void Log( const char* data )
{
    Logger.getInstance().DoRealLog( data );
}

This is a fine logger interface, all the power of your singleton, limited fuss.

Is there a possible memory leak when I'm accessing the class using. What about singleton destruction?

There is a risk of double instantiation if you're lazily allocating the singleton on the heap in a multithreaded environment. This would cause surplus instances to leak.

There are somewhat similar risks if you use a static local: http://blogs.msdn.com/b/oldnewthing/archive/2004/03/08/85901.aspx

In my opinion explicit construction when execution starts is the best option, but whether it is done in the data segment (with a static variable in a class or global variable) or the heap is very unlikely to matter in your case. If you do allocate it on the heap you should also delete it.