tags:

views:

156

answers:

7

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.

A: 

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;
Brian Hooper
If the exception is caught, then m_Instance would still be null, and the macro would expand into code which dereferences a null pointer, causing further problems.
Pete Kirkham
What do you do if the allocation fails? Log the error?
Mike Seymour
A good question; what DO you do when the error logging system fails?
Brian Hooper
+8  A: 

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.

DeadMG
+1: I don't understand why everyone wants to use this awful singleton pattern.
ereOn
@ereOn: Obviously Qt leads you into this direction... Also, Singleton seems to be an easy design pattern and therefore beginners remember and use it.
ur
I don't understand why some say singleton is good to use sometimes and others say its horrible most of the time. *confused*
InsertNickHere
@InsertNickHere: Singleton is a terrible design pattern because you run into all sorts of problems when you hit threading and stuff, and there's flat out no need for it if you make an application instance like every other smart person.
DeadMG
@ur: I'm not sure to understand why `Qt` would lead us into this direction ? Do you refer to `QApplication` ?
ereOn
@Insert: Maybe [jalfs post](http://jalf.dk/blog/2010/03/singletons-solving-problems-you-didnt-know-you-never-had-since-1995/) on the matter helps to understand its dislike.
Georg Fritzsche
:) asfor why some say it's "good to use sometimes", the reason is ignorance. It *looks* simple. it was even recommended in a book that a lot of people treat like the Bible of software development. So yeah, if you don't think too much about it, or if you're not aware that the problems you're struggling with *could have been avoided at all*, the singleton looks great.
jalf
@DeadMG what is the application instance pattern? A google search on it didn't lead to many matches and I could not figure exactly what was meant. My assumption would be something akin to either a single, globally accessible application instance (application singleton and the only case where we use a singleton), or an application object that's passed around through the system to every function that could ever potentially use it.
I wouldn't like to make an authorative statement and say that singleton is never useful - but I've never come across a situation where it's useful, yet I have come across all the situations commonly used as an example of where singleton is useful.
Joe Gauterin
@stinky472: It's an application object that you pass around, and it's really useful because if you ever come to do more than one thread, your code doesn't horrifically die because you can create a new app instance (or more commonly, one main app instance and one per-thread app instance). In addition, it makes everything much more flexible- what if someone later wanted more than one Logger? They'd have to rewrite ALL your code that uses a Logger. Use an app instance -> no problem. The other thing is that you put all your otherwise static data there, replacing ALL of it with ONE reference.
DeadMG
@DeadMG ah that makes sense. We do something similar and tend to abhor singletons, but with thread-local storage (we don't pass it around everywhere). I'm not sure if that's good enough, but it avoids the parallelism issue. We developed a proprietary way to do very fast thread-local storage - we can fetch data associated with the current thread in several clock cycles as opposed to hundreds like QThreadStorage which served as our primary comparison. It's still like thread-global access and has its associated downsides, but the upside is that we don't have to modify our public interfaces.
@DeadMG something akin to, Application where fetch_app will fetch the application-associated data for the current thread. It does still have that problem that if we ever need more than one application state object per thread, we'd have to rewrite a lot of code.
what do you recommend to use instead of singleton? I need to the following behavior:- Global access (without need to create new instance for each class)- Should maintain its state (for example, I open file handle only once during the first call and then use the same handle during the lifetime)- Should be possible to add some logic inside the class (e.g. in my settings object there is a QTimer that periodically checks changes in some files etc.)
itsme1
@stinky472: There's still no reason to ensure that only one of the class exists, at all, and no advantage to it. [email protected]: Make an Application class, insert data that you would have made global, instantiate once, pass around. Voila.
DeadMG
@ereOn: people use the Singleton antipattern because it makes it easy to cobble together a small project without worrying about dependencies. It usually takes a while for the project to grow large enough that the tight coupling and (in C++) object lifetime and concurrency issues become a serious nuisance. By that time, it may be politically impossible to change the design.
Mike Seymour
@stinky: interesting. Can you post a description somewhere of how that TSS is implemented?
jalf
@itsme1: first, does it really need to be global? What about just creating a single instance at the start of your program, and then passing a reference around to every object that needs to use it?Second, if it *does* have to be global, make it a global. Not a singleton. A single object declared outside any and all functions.
jalf
More ideas of singleton poorness: If you ever want to convert your code into a library to be re-used in other projects, they will expect that it does not use globals.
DeadMG
@jalf it's a lot of profiler-aided, hand-tuned, evil code. Basically we use OS-specific ways to fetch the current thread ID. We tested this on linux, OSX, and Windows and fetching IDs was very fast (might even be using FS:18 on intel machines for all I know). From that we use a crude hash algorithm on a fixed array to determine where in the array the thread is associated. The size of the array is hard-coded and never changes. We use atomic test and set to determine if the hash location is occupied, and if so, whether it matches the thread ID [...]
@jalf thanks for the reply. It's not mandatory to be global, it just makes things easier (no need for long reference chains e.g. new Parent(logger) -> new Child(logger) -> new ChildChild(logger) etc.)
itsme1
@jalf [...] If it does and it matches, we return that data. If it's occupied and doesn't match, we use the slow exceptional case of using QThreadStorage. If the position wasn't occupied, we instantiate the data and store it in that position. The result is that for non-exceptional cases, we just fetch the thread ID, compute a quick hash value from it, apply one atomic test and set, and return the pointer associated.
@itsme1: That's easier than a singleton. In addition, you only need one reference to one object- it can store all the global data you need.
DeadMG
@jalf: for thread-local storage, you could use `boost::thread_specific_ptr`, or find a compiler that implements the `thread_local` storage class of C++0x.
Mike Seymour
@jalf [...] actually when I write out what we're doing, it sounds like it should take a dozen cycles instead of several (that would still be okay). We did some performance tests just on this directly as opposed to QThreadStorage and it showed several cycles per iteration as opposed to hundreds, but it wasn't a real world test (hot caches, e.g.) and there are all kinds of optimizations the compiler could have made to make that test fast. Nevertheless, we use thread-local storage quite a bit in our application and replacing it with this method sped up the real world performance tests nicely.
@DeadMG: Thats true (looks ground). I'm not really pro with the C++ pointers/references so I want to ask the following. If I create the logger in parent.cpp (pointer declared in .h) like this: *log = new Logger();. In child.h I have another pointer which i set in constructor like Child::Child(Logger *log) : m_Logger(log) {} is that the correct way? What about memory management, what happens when child is destroyed? Would it be better to use stack instead (how to store logger in child so that it won't be destroyed with the child).
itsme1
@DeadMG That's true. To try to mitigate this potential future problem, we do pass the application object around a lot and try not to depend on this thread-specific storage too much, but we don't thoroughly pass this application object around everywhere and that's where this TSS method comes in handy. Had we had a stricter design policy to make every function/notification system accept this application object, we probably could have avoided that completely, but it's hard to enforce such policies on everyone.
@itsme1: It's simple. You make an object, normally first item in main(). You put the Logger instance in it. Then you take a reference to it and stick it wherever you need. @stinky472: Sounds to me like you don't have a problem, except that your other coders suck.
DeadMG
@Mike Yeah, I've been using `boost::thread_specific_ptr` so far in the cases where I needed TSS, I was just curious about @stinky's optimized implementation. I might experiment with doing something like it, as I've got one project where the performance impact of TSS is a bit painful (and where, unfortunately, its use can't be avoided)
jalf
@jalf: You should also check for your compiler. Some like MSVC offer their own TSS implementations.
DeadMG
+2  A: 

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.

Pete Kirkham
+1 for the null object pattern - I like it.
Space_C0wb0y
+3  A: 

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);
}
The std::nothrow won't stop the constructor from throwing, it only stops 'operator new' from throwing.
Joe Gauterin
@Joe good point. I was thinking of this problem too much in a vacuum. I will modify this accordingly.
@itsme1: the memory leak could be a problem it you're testing for memory leaks. Allowing "harmless" leaks has the same effect as allowing "harmless" compiler warnings: the log will contain so many that you might not spot new, possibly harmful, ones.
Mike Seymour
@Mike +1. I was trying to say the same thing, but you worded it in a much clearer way.
+1  A: 

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!");
adf88
A: 

(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
    ...
  }
}
ur
WHy put the HACK there, when there is a much cleaner place in the macro `#define LOG (Logger::Instance()== NULL ? Logger::DevNull : Logger::Instance())->Log`. The idea is that `Logger::DevNull->Log(foo)` discards foo.
MSalters
@MSalters: Because it's broken in Logger class, so fix it in Logger class.
ur
A: 

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.

itsme1