Skip to main content
Code Review

Return to Answer

added 1441 characters in body
Source Link
Justin
Justin
  • 3.3k
  • 1
  • 21
  • 39
#pragma once

class MySingleton {
public:
    static MySingleton &get_instance();
    
    // simply define an interface
    virtual void do_something() = 0;
    virtual void set_something(int i) = 0;
};

namespace detail {
    struct MySingletonImpl final : MySingleton {
        int value;
        
        void do_something() override;
        
        void set_something(int i) override;
    private:
        // We have to hide it now because it was previously
        // hidden by virtue of being in a cpp file
        MySingletonImpl() = default;
        friend MySingleton &MySingleton::get_instance();
    };
}

MySingleton &MySingleton::get_instance() {
    static detail::MySingletonImpl instance;
    return instance;
}

#pragma once

class MySingleton {
public:
    static MySingleton &get_instance();
    
    // simply define an interface
    virtual void do_something() = 0;
    virtual void set_something(int i) = 0;
};

namespace detail {
    struct MySingletonImpl final : MySingleton {
        int value;
        
        void do_something() override;
        
        void set_something(int i) override;
    };
}

MySingleton &MySingleton::get_instance() {
    static detail::MySingletonImpl instance;
    return instance;
}

#pragma once

class MySingleton {
public:
    static MySingleton &get_instance();
    
    // simply define an interface
    virtual void do_something() = 0;
    virtual void set_something(int i) = 0;
};

namespace detail {
    struct MySingletonImpl final : MySingleton {
        int value;
        
        void do_something() override;
        
        void set_something(int i) override;
    private:
        // We have to hide it now because it was previously
        // hidden by virtue of being in a cpp file
        MySingletonImpl() = default;
        friend MySingleton &MySingleton::get_instance();
    };
}

MySingleton &MySingleton::get_instance() {
    static detail::MySingletonImpl instance;
    return instance;
}
added 1441 characters in body
Source Link
Justin
Justin
  • 3.3k
  • 1
  • 21
  • 39

Do note that this will likely lead to a virtual table lookup at runtime, unless you instead declare the MySingletonImpl in the header file and define get_instance() in the header file:

my-singleton.h

#pragma once

class MySingleton {
public:
    static MySingleton &get_instance();
    
    // simply define an interface
    virtual void do_something() = 0;
    virtual void set_something(int i) = 0;
};

namespace detail {
    struct MySingletonImpl final : MySingleton {
        int value;
        
        void do_something() override;
        
        void set_something(int i) override;
    };
}

MySingleton &MySingleton::get_instance() {
    static detail::MySingletonImpl instance;
    return instance;
}

my-singleton.cpp

#include "my-singleton.h"
#include <iostream>
    
void detail::MySingletonImpl::do_something() {
    std::cout << value << '\n';
}

void detail::MySingletonImpl::set_something(int i) {
    value = i;
}

The compiler will then be likely to be able to optimize it to the correct function call, especially if get_instance was instead detail::MySingletonImpl &get_instance(), but I don't think that will cause any problems.

Either way, the performance penalty will probably be small, so it's probably not worth your time.

Do note that this will likely lead to a virtual table lookup at runtime, unless you instead declare the MySingletonImpl in the header file and define get_instance() in the header file:

my-singleton.h

#pragma once

class MySingleton {
public:
    static MySingleton &get_instance();
    
    // simply define an interface
    virtual void do_something() = 0;
    virtual void set_something(int i) = 0;
};

namespace detail {
    struct MySingletonImpl final : MySingleton {
        int value;
        
        void do_something() override;
        
        void set_something(int i) override;
    };
}

MySingleton &MySingleton::get_instance() {
    static detail::MySingletonImpl instance;
    return instance;
}

my-singleton.cpp

#include "my-singleton.h"
#include <iostream>
    
void detail::MySingletonImpl::do_something() {
    std::cout << value << '\n';
}

void detail::MySingletonImpl::set_something(int i) {
    value = i;
}

The compiler will then be likely to be able to optimize it to the correct function call, especially if get_instance was instead detail::MySingletonImpl &get_instance(), but I don't think that will cause any problems.

Either way, the performance penalty will probably be small, so it's probably not worth your time.

Source Link
Justin
Justin
  • 3.3k
  • 1
  • 21
  • 39

As I mentioned in the other answer, this solution for testing singletons is easy to use wrong. Any cached instance of the singleton makes your unit tests wrong. However, there is another way to make your singletons unit testing friendly: have the singleton implement an interface.

A real world example of this is Eclipse's Ecore. Ecore is a Java modeling framework which generates code for you. Each package it generates is equipped with a MyPackageFactory - effectively a singleton that you use with MyPackageFactory.eINSTANCE.createFoo(). However, MyPackageFactory is actually an interface; the singleton is implemented in MyPackageFactoryImpl. What this means is that you can inject the singleton into your classes, allowing you to mock for unit tests as well.

As a rough example:

my-singleton.h

#pragma once

class MySingleton {
public:
    static MySingleton &get_instance();
    
    // simply define an interface
    virtual void do_something() = 0;
    virtual void set_something(int i) = 0;
};

my-singleton.cpp

#include "my-singleton.h"
#include <iostream>

struct MySingletonImpl final : MySingleton {
    int value;
    
    void do_something() override {
        std::cout << value << '\n';
    }
    
    void set_something(int i) override {
        value = i;
    }
};

MySingleton &MySingleton::get_instance() {
    static MySingletonImpl instance;
    return instance;
}

It can then be used in a unit-testing friendly way like so:

uses-singleton.h

#pragma once

#include "my-singleton.h"

struct UsesSingleton {
    MySingleton *singleton;
    int i;
    
    explicit UsesSingleton(MySingleton &singleton, int i)
        : singleton{ &singleton }
        , i{ i }
    {}
    
    void do_something() {
        singleton->do_something();
        singleton->set_something(i);
        singleton->do_something();
    }
};

main.cpp

#include "uses-singleton.h"

int main() {
    UsesSingleton myStruct{ MySingleton::get_instance(), 10 };
    myStruct.do_something();
}

uses-singleton-test.cpp -- (using googlemock just as an example. also this is untested)

#include "gtest/gtest.h"
#include "gmock/gmock.h"

class MySingletonMock : public MySingleton {
public:
    MOCK_METHOD0(do_something, void());
    MOCK_METHOD1(set_something, void(int))
};

TEST(UsesSingletonTests, SampleTest) {
    MySingletonMock singleton;

    {
        ::testing::InSequence dummy;

        EXPECT_CALL(singleton, do_something());
        EXPECT_CALL(singleton, set_something(10));
        EXPECT_CALL(singleton, do_something());
    }

    UsesSingleton myStruct{ singleton, 10 };
    myStruct.do_something();
}