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\$

closed as off-topic by Jamal Feb 9 '14 at 3:53

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Your question must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After getting your code to work, you may edit this question seeking a review of your working code." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 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?

\$\endgroup\$
4
\$\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

\$\endgroup\$

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