Implementation of a singleton class with atomic members

Question

I'm trying to implement a class to track the performances of a multithreaded code and am running into the following issue.

The tracker should have a single instance, thus the singleton pattern. In order to count object creation and function execution accross all the threads, I thought that using atomic member would be a good idea. But I can't seem to figure out a correct implementation.

This is a minimal code of what I wand to do:

#include <atomic>
#include <iostream>

class Counter
{
public:
    static Counter& instance()
    {
        return instance_;
    };

    void increment()
    {
        counter_++;
    };

private:
    Counter ()
    {
        std::cout << "ctor counter" << std::endl;
    };

    ~Counter ()
    {
        std::cout << "counter_: " << counter_ << std::endl;
        std::cout << "dtor counter" << std::endl;
    }

    static Counter instance_;
    std::atomic<int> counter_{0};
    //int counter_ = 0;
};

Counter Counter::instance_ = Counter();

int main(void)
{
    Counter::instance().increment();
    Counter::instance().increment();
    Counter::instance().increment();

    return 0;
}

If the counter_ variable is an int, it works fine, but would not be thread safe. If it is an atomic<int>, then the compiler tells me this:

   g++ foo.cc
foo.cc:34:38: error: use of deleted function 'Counter::Counter(const Counter&)'
 Counter Counter::instance_ = Counter();
                                      ^
foo.cc:4:7: note: 'Counter::Counter(const Counter&)' is implicitly deleted because the default definition would be ill-formed:
 class Counter
       ^~~~~~~
foo.cc:4:7: error: use of deleted function 'std::atomic<int>::atomic(const std::atomic<int>&)'
In file included from foo.cc:1:0:
/usr/include/c++/7/atomic:668:7: note: declared here
       atomic(const atomic&) = delete;
       ^~~~~~

I'm not sure I quite understand the problem. Any explanation/solution would be much appreciated.

Cheers

Answer 1

You could simplify by having std::atomic<int> counter_{0}; simply be a static class member instead of part of each instance. (Since you're ensuring that there's only one instance the class.)

Or if you're only using your "singleton" that way returning a reference to a static object, just make all of its members static so you don't need to even get a pointer to that single instance of it in the first place. Then it can just be a glorified namespace{} with public and private static member functions. I think the only point of a "singleton" is to delay initializing it until after static initialization by using a function-scoped static with a non-constant initializer, but you're not doing that.

---

The actual problem is in the copy-constructor of your class, which your static initializer uses the way you've written it. It constructs a temporary Counter(); and then copies it to static instance_ variable.

You can compile as C++17 where eliding of that copy is guaranteed (works on Godbolt with g++ -std=gnu++17 with your source untouched), or you can rewrite the initializer

Counter Counter::instance_;    // default construct
Counter Counter::instance_{};  // explicitly default construct

Both of those work with g++ -std=gnu++11