CODEWITHSUNDEEP
c++c++11
Jan Moritz
Jan Moritz

Reputation: 2215

c++11 Singleton is creating new instances

This is my singleton

class MySingleton {
private:
    int myNumber;
public:
    static MySingleton &get() {
        static MySingleton instance;
        return instance;
    }

    int getMyNumber() const {
        return myNumber;
    }

    void setMyNumber(int myNumber) {
        MySingleton::myNumber = myNumber;
    }
};

and this is the code to test it

MySingleton t1 = MySingleton::get();
t1.setMyNumber(6);
printf("t1: %d \n", t1.getMyNumber());

MySingleton t2 = MySingleton::get();
printf("t2: %d \n", t2.getMyNumber());

Now the output I get is

t1: 6 
t2: 0

but te expected result would be

t1: 6 
t2: 6

It looks like MySingleton::get() is creating a new instance, something it shouldn't do of course.

Upvotes: 0

Views: 155

Answers (4)

Edgar Rokjān
Edgar Rokjān

Reputation: 17483

The reason is that you create a copy of the instance in these lines:

MySingleton t1 = MySingleton::get();
MySingleton t2 = MySingleton::get();

and change the copy.

To make it work you have to add a reference:

MySingleton& t1 = MySingleton::get();
MySingleton& t2 = MySingleton::get();

Also note, that the classic singleton design assumes deleted copy constructor and copy assignment operator to avoid such bugs:

class MySingleton {
    private:
        // ...
    public:
        // ...
        MySingleton(const MySingleton&) = delete;
        void operator=(const MySingleton&) = delete;
};

P.S. You might find this discussion useful. It shows the implementation details of a singleton pattern with C++.

Upvotes: 2

Gal Menash Ofri
Gal Menash Ofri

Reputation: 21

You declare both t1 and t2 as instances of the class rather than references to the class.

The get function is implemented correctly but then each instance is constructed upon assignment. Note that making your constructor private would have prevented this bug at compilation.

I suggest going through a tutorial on how to implement Singletons in c++ to learn some good practices, or simply using an existing well-made impelmentation.

Upvotes: 0

The Techel
The Techel

Reputation: 873

You are copying the singleton instead of obtaining a reference:

MySingleton &t1 = MySingleton::get();

Mark the copy- an move-assignment and -constructors as delete and make the constructor and destructor private:

class MySingleton
{
public:
    MySingleton(const MySingleton&) = delete;
    void operator=(const MySingleton&) = delete;

    MySingleton(MySingleton&&) = delete;
    void operator=(MySingleton&&) = delete;

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

};

Note that you probably just want a global variable instead of a singleton.

Upvotes: 0

Matthias247
Matthias247

Reputation: 10426

In your case get() isn't creating the copy. The assignment to t1 and t2 is doing it. To avoid it change those to references: MySingleton& t1 = MySingleton::get().

And in order to avoid accidental errors remove the copy and assignment operators:

MySingleton(const MySingleton& other) = delete;
MySingleton& operator=(const MySingleton& other) = delete;

Upvotes: 6

Related Questions