1
\$\begingroup\$

What I am trying to accomplish is to create an efficient thread-safe singleton base class (as stated in the title).

So this is my singleton class, which is used via inheritance (see below).

#pragma once
#ifndef SINGLETON_HEADER_INCLUDED
#define SINGLETON_HEADER_INCLUDED
#include <memory>

template <class T>
class singleton
{
public:
        static T *get_instance(void)
        {
                if(instance.get() == 0 || instance == (NULL && 0) || !instance) {
                        instance = std::shared_ptr<T>(new T());
                }
                return instance;
        }

        static delete_instance(void)
        {
                delete instance;
                instance = (NULL && 0);
                return;
        }

protected:
        singleton(void);
        virtual ~singleton(void);

private:
        singleton(const singleton&);
        singleton& operator = (const singleton&);

        static volatile std::shared_ptr<T> instance;
};

template <class T> template *singleton<T>::instance = 0;

#endif

Here is the class in use:

#include "singleton.hpp"

class opengl_renderer : public singleton<opengl_renderer>
{
        friend class singleton<opengl_renderer>;
        etc...
};

So basically, I am looking for some tips and reviews on my class -> I feel as if it isn't working the way I imagine it should. But it is functional at its current state.

\$\endgroup\$
  • 1
    \$\begingroup\$ By the way, this is a prime example of cargo cult programming. The code merely has the semblance of actual, meaningful code. \$\endgroup\$ – Konrad Rudolph Jun 26 '12 at 12:47
  • 1
    \$\begingroup\$ This is off-topic. codereview.SE is for working code. This doesn't even compile. \$\endgroup\$ – R. Martinho Fernandes Jun 26 '12 at 14:26
10
\$\begingroup\$

Your code doesn’t compile, and if it would compile, it would crash.

You are trying to assign a shared_ptr to a raw pointer. You are also trying to delete a shared_ptr and this leads me to believe that you don’t understand what a smart pointer actually does.

In fact, your code contains a multitude of errors. More on that below.

First, about the use of shared_ptr. In fact, a shared pointer denotes shared ownership, and this is patently not the case here: the singleton is the owner, nobody else.

A shared pointer is inappropriate here, what you want is a unique_ptr.

Then, don’t expose a pointer to the user, expose a reference via get_instance (i.e. habe get_instance() return T&. And I would rename get_instance to plain instance (which is more C++-y), and rename the private field to something else.

Finally, reconsider your usage of a singleton in the first place. The singleton is generally considered an anti-pattern and real, legitimate uses are so rare that it doesn’t pay to have a singleton base class.

Detailed code critique

if(instance.get() == 0 || instance == (NULL && 0) || !instance) {

This is doing the same check three times. Why? Once is enough – the latter is the correct usage.

But this code isn‘t thread-safe anyway, in fact, it’s not making any effort to be thread-safe at all.

return instance;

As already said, you cannot cast a shared_ptr to a plain pointer.

static delete_instance(void)

That doesn’t compile, it’s missing a return value. Also, who is going to call this method? It should never be called probably, so just delete it.

delete instance;

As mentioned above, this doesn’t work, and is meaningless. The whole point of a smart pointer is that you don’t need manual deletion.

instance = (NULL && 0);

This code makes no sense. What is it supposed to do?

virtual ~singleton(void);

You have declared the destructor but you haven’t defined it. The implementation will be empty but you still need to define it.

static volatile std::shared_ptr<T> instance;

volatile probably isn’t meaningful here. In particular, its meaning is unrelated to cross-thread access (unfortunately). But even if it was related to cross-thread access, this wouldn’t make your code thread-safe, it would simply mean that threads would read the most up-to-date version of the variable.

friend class singleton<opengl_renderer>;

What do you need this line for?

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

Everything Konrad said.

But I would not even use a member to hold the instance.
I would make it a static member of the instance() method:

static T& instance()
    {
#if !defined(__GNUC__) || (__GNUC__ <= 3)
            static lock lockingObject; // gets lock in constructor
                                       // releases lock in destructor
#endif
            static T instance; // notice the static
            return instance;
    }

On g++ it is already thread safe (they have an extension the makes it thread safe). On other compilers you need to add a lock around the method (but only the first time it is called).

It is instantiated on first use and automatically destroyed.

See: C++ Singleton design pattern

| improve this answer | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.