1

During re-factorization of singleton class to be thread safe (fallowing Herb Sutter advice how to write proper double-check locking) I came across problem with my version of compiler (gcc 4.4.7 with --std=c++0x flag) supporting atomics but not supporting nullptr.

Current code

class SingletonClass {
   public:
SingletonClass* getInstance() {
    if (instance == NULL) {
        instance == new SingletonClass();
    }
    return instance;
}

   private:
SingletonClass() = default;
~SingletonClass() = default;

static SingletonClass* instance;};

What I would like to achive

#include <cstdatomic>
#include <mutex>

class SingletonClass {
   public:
SingletonClass* getInstance() {
    if (instance == NULL) {
        std::lock_guard<std::mutex> lock (m);
        if(instance == NULL){
           instance = new SingletonClass();
        }
    }
    return instance;
}

   private:
 SingletonClass() = default;
~SingletonClass() = default;

static std::atomic<SingletonClass*> instance;
static std::mutex m;
};

But this gives me error saying that there is no operator for comparing atomic ptr to NULL

main.cpp: In member function ‘SingletonClass* SingletonClass::getInstance()’:
main.cpp:7: error: ambiguous overload for ‘operator==’ in ‘SingletonClass::instance == 0l’
main.cpp:7: note: candidates are: operator==(void*, void*) <built-in>
main.cpp:7: note:                 operator==(SingletonClass*, SingletonClass*) <built-in>

Since I can't either compare instance ptr to NULL or use nullptr how do I work around it and check whether it is initialized or not ?

Roland Puntaier
  • 3,250
  • 30
  • 35
scadder
  • 11
  • 2

2 Answers2

1

You can use implicit conversion of pointer to bool:

SingletonClass* getInstance() {
    if (instance.load()) {
        std::lock_guard<std::mutex> lock (m);
        if(instance.load()){
           instance.store(new SingletonClass());
        }
    }
    return instance;
}

See it online

Note that implicit conversion from std::atomic<SingletonClass*> to SingletonClass* is possible, but ambiguous in this context. Also, the assignment is ambiguous itself, so store() call was added.


However, perhaps the solution is simpler - why do you need std::atomic at all? You are already locking access to the stored pointer, so you are safe:

#include <mutex>

class SingletonClass {
public:
    SingletonClass* getInstance() {
        std::lock_guard<std::mutex> lock (m);
        if (instance == NULL) {
            instance = new SingletonClass();
        }
        return instance;
    }

private:
    SingletonClass() = default;
    ~SingletonClass() = default;

    static SingletonClass* instance;
    static std::mutex m;
};

std::atomic is used for lockfree access (or at least "hidden lock" access). I can't think of a reason to use both together off the top of my head.

Mutex here is pretty much required - you want to lock whole function as a critical section, or else two threads could create two singleton objects and one would be leaked.

Yksisarvinen
  • 18,008
  • 2
  • 24
  • 52
  • I agree with @Yksisarvinen on this actually. Once you have the lock, the `atomic` isn't needed and just complicates things. – Kevin Anderson Mar 10 '20 at 17:20
0

Edit: I think Yksisarvinen's answer above is better. You already have the lock, don't bother anymore. Leaving my old answer below.

I'm not sure about the specific GCC version, but if <atomic> is there theoretically this should work:

SingletonClass* getInstance() {
    if (instance.load() == NULL) {
        std::lock_guard<std::mutex> lock (m);
        if(instance.load() == NULL){
            instance = new SingletonClass();
        }
    }
    return instance;
}

As per the documentation, load() should give you what you want, the pointer itself to compare to, which should work with NULL.

Good luck on old compilers. I've had to do that too on occasion due to customer requirements.

Kevin Anderson
  • 6,850
  • 4
  • 32
  • 54
  • 1
    My answer suggests going straight with implicit conversion to bool, but yours [works just as well](https://wandbox.org/permlink/Rt4I58M29rHtK0Xi) . There is an issue with assignment, but it's easily resolved with a `store()` call – Yksisarvinen Mar 10 '20 at 15:43
  • Thank you very much, this is what I was looking for! – scadder Mar 10 '20 at 15:49