Search code examples
c++pimpl-idiomabstract-factory

Runtime error implementing Abstract Factory PIMPL Idiom in C++


when trying to implement the abstract factory under the PIMPL idiom I get a runtime error when trying to acquire an object from outside the Factory scope. (See section commented with "Runtime Error" in Main. This occurs when calling acquireInterface() method from the public class, which calls acquireInterface() from implementation). However this doesn't happen when acquireInterface() from the testFactory() function inside the implementation class (see "testFactory()" function). Any advise? Tried in MinGW 4.8 and VC++11 with RTTI enabled.

Thanks in advance.

--- File: IType.hpp ---

#ifndef ITYPE_HPP_
#define ITYPE_HPP_


class ITYPE {
public:
    ITYPE() {
        std::cout << "ITYPE()" << std::endl;
    };
    virtual ~ITYPE(){
        std::cout << "~ITYPE()" << std::endl;
    };
    virtual void hello() = 0;
};


#endif /* ITYPE_HPP_ */

--- File: Factory.hpp ---

#ifndef FACTORY_HPP
#define FACTORY_HPP

#include <memory>
#include <iostream>

#include "IType.hpp"

class Factory {
public:
    Factory();
    ~Factory();

    void testFactory();

    ITYPE* acquireInterface(const char* type);

private:
    class Impl;
    std::unique_ptr<Impl> m_pimpl;
};

#endif //FACTORY_HPP

--- File: FactoryImpl.cpp ---

// Implementations
// ----------------------------------------------------------------------

class ConcreteA : public ITYPE {
public:
    ConcreteA(){ std::cout << "ConcreteA()" << std::endl; }
    ~ConcreteA(){ std::cout << "~ConcreteA()" << std::endl; }

    void hello() { std::cout << "A says Hello" << std::endl; }
};

class ConcreteB : public ITYPE {
public:
    ConcreteB(){ std::cout << "ConcreteB()" << std::endl; }
    ~ConcreteB(){ std::cout << "~ConcreteB()" << std::endl; }
    void hello() { std::cout << "B says Hello" << std::endl; }
};


// ----------------------------------------------------------------------


template<typename Type> ITYPE* createType()
{
    return new Type();
}

/**
 * @brief Abstract Factory for ITYPES.
 */
class Factory::Impl {
    public:
        /**
         * @brief Constructor
         * @details Implementations to be added here using function addType()
         */
        Impl() {
            addType<ConcreteA>("A");
            addType<ConcreteB>("B");
        };

        ITYPE* acquireInterface(const char* type)
        {           
            std::cout << "Acquiring interface for " << type << "..." << std::endl;
            Impl::map_type::iterator iter = m_map.find(type);
            return iter->second();      
        }

    // THIS WORKS
    void testFactory()
    {
        ITYPE* iA = acquireInterface("A");
        iA->hello();
        delete iA;

        ITYPE* iB = acquireInterface("B");
        iB->hello();
        delete iB;
    }

    private:
        /** @brief Adds a type to the Abstract Factory
         *  @param componentName short string (no spaces) to identify implementation */
        template<typename Type>
        void addType(const char* componentName) {
            ComponentFactoryFuncPtr function = createType<Type>;
            m_map.insert(std::make_pair(componentName, function));
        };

public:
        /**
         * @brief Abstract factory constructor function pointer
         */
        typedef  ITYPE* (*ComponentFactoryFuncPtr)();

        /**
         * @brief Type for map holding type identifier / constructor function
         */
        typedef std::map<const char*, ComponentFactoryFuncPtr> map_type;

private:
     map_type m_map;    /**< map holding type identifier / constructor function */
};

Factory::Factory() : m_pimpl(new Impl()) { }

Factory::~Factory() { }

void Factory::testFactory()
{
    m_pimpl->testFactory();
}

ITYPE* Factory::acquireInterface(const char* type)
{
    return m_pimpl->acquireInterface(type);
}

--- main ---

#include <iostream>
#include <memory>
using namespace std;

#include "Factory.hpp"

int main() 
{
    {
        Factory f;

        // OK
        std::cout << "This works:"  << std::endl;
        f.testFactory();

        // Runtime error (acquireInterface("A") returns NULL ptr)
        ITYPE* iA = f.acquireInterface("A");
        iA->hello();
        delete iA;

        ITYPE* iB = f.acquireInterface("B");
        iB->hello();
        delete iB;
    }

    return getchar();
}

Solution

  • One bad thing in your code is this:

    typedef std::map<const char*, ComponentFactoryFuncPtr> map_type;
    

    The major issue is that you cannot guarantee that const char* literals will have the same address, even though the literal is the same.

    Your code attempts this:

    ITYPE* iA = f.acquireInterface("A");
    

    There is no guarantee that the string-literal "A" has a pointer value the same as the "A" you set up your map with. Thus the behavior is undefined as to what will happen.

    If the goal of the map key is to have a string, then use a string. You now have total control of what the key represents, instead of a const char * where you don't really know how the compiler will handle string literals. All you really do know is that "A" is a string-literal, but that's all you really can know.

    The fix should be this:

    typedef std::map<std::string, ComponentFactoryFuncPtr> map_type;