Jump to content
  • Advertisement
Sign in to follow this  
Timkin

My pluggable factory is broken! 8(

This topic is 4017 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Here's an overview of my pluggable factory...
// EntityFactory.h
class EntityFactory
{
public:
    typedef std::map<OBJ_TYPE,EntityFactory*>	FactoryMap;

protected:

    static FactoryMap	m_registry;
    virtual BaseEntity*	Create()=0;

public:
    EntityFactory(OBJ_TYPE _type);
    static BaseEntity*  Construct(OBJ_TYPE _type);

};

// EntityFactory.cpp
EntityFactory::FactoryMap EntityFactory::m_registry;

EntityFactory(OBJ_TYPE _type){
    m_registry.insert(std::make_pair(_type,this));
}

BaseEntity*  Construct(OBJ_TYPE _type){
    EntityFactory::FactoryMap::iterator mapIter = m_registry.find(_type);
    if (mapIter!=EntityFactory::m_registry.end()){
        EntityFactory* pObjectFactory = (*mapIter).second;
        return pObjectFactory->Create();
    }
    else
        return NULL;
}


// Object.h
class Object : public BaseEntity{...};


class ObjectFactory : public EntityFactory
{
public:
    ObjectFactory():EntityFactory(IDC_OBJECT1){}
private:
    BaseEntity* Create() const{
        Object* pObj = NULL;
        try{
            pObj = new Object;
        }
        catch(...){};
        return pObj;
    }

    static const ObjectFactory register_this;
};



but it's not working. When I try to create an object using a call such as
Object* pObj = reinterpret_cast<Object*>(EntityFactory::Construct(IDC_OBJECT1));
I get back a NULL pointer... the reason being that EntityFactor::m_registry is empty and so the m_registry.find(_type) call is failing. So obviously the ObjectFactory is not creating its base type (EntityFactory) in its constructor call. I've taken this design from the article "Why Pluggable Factories Rock My World"... so what did I do wrong? Thanks for any help you can provide. Cheers, Timkin [Edited by - Timkin on June 23, 2007 3:17:36 AM]

Share this post


Link to post
Share on other sites
Advertisement
You did remember to define the static member (as opposed to just declaring it) somewhere (presumably in Object.cpp), right?

const ObjectFactory ObjectFactory::register_this;

Share this post


Link to post
Share on other sites
Quote:
Original post by joanusdmentia
You did remember to define the static member (as opposed to just declaring it) somewhere (presumably in Object.cpp), right?

const ObjectFactory ObjectFactory::register_this;


I had tried that, but it wasn't possible to compile this because ObjectFactory is an abstract class (I get a "ObjectFactory: Cannot instantiate abstract class" error).

Clearly the problem is one of no instantiation... otherwise the ObjectFactory constructor would be called and the registry entry would be made... so without the above instantiation doesn't occur... but it can't occur while the class is abstract... and I cannot see what is different between what I've done and what the article did.

Share this post


Link to post
Share on other sites
ObjectFactory shouldn't be abstract. If the code you posted is the code you're actually using, the reason ObjectFactory is abstract is that Create() has a different signature to Create() in EntityFactory; it differs by const.

Share this post


Link to post
Share on other sites
Thanks guys... it was the omission of 'const' (I must be blind) that was causing the compilation problem...now I just need to work out what's causing the fatal error on startup on the call

EntityFactory::m_registry.insert(std::make_pair(_type,this));

reporting an access violation. 8( Ah well, at least that's one problem out of the way... thanks...

Timkin

Share this post


Link to post
Share on other sites
I use a very simple abstract factory which you can use and abuse if you so feel :)

///////////////////////////////////////////////////////////////////////////////
/// abstract_factory.h
/// Allows a user to register types and a key for the type(can be enums ints
/// strings). There are two register functions one which takes a create
/// function which must match the function signature:
/// std::auto_ptr<Base> (*func)()
/// this allows the user to be able to construct the object in the manner
/// which they require .
/// If a create function is not supplied then the default will be used which
/// just returns an auto pointer to the base class.
///
///
/// @author Liam Devine, Dan Oppenheim and Richard Smith (LDR) @date 08/03/2007
///////////////////////////////////////////////////////////////////////////////

#ifndef ABSTRCT_FACTORY_H_
# define ABSTRCT_FACTORY_H_

# include <map>
# include <string>
# include "types.h"
# include <algorithm>
# include <memory>

namespace LDR
{
template<typename Base, typename Class>
inline std::auto_ptr<Base> abstract_creater()
{
return std::auto_ptr<Base>(new Class);
}

template<typename Base,typename Id>
class Abstract_factory
{
public:
typedef std::auto_ptr<Base> (*func)();

Abstract_factory(){}
//search the map and look for the create function for this key, if found return the return value
//of the function otherwise return null pointer
std::auto_ptr<Base> create(Id key)
{
iter i;
if(( i = m_creater.find(key) ) != m_creater.end() ){ return i->second(); }
else
{
throw std::runtime_error("could not find the key in the factory");
}
}
//register a key and a create function for this key
bool register_key(Id key, func function)
{
iter i;
if ( ( i = m_creater.find(key) ) == m_creater.end() )
{
m_creater[key] = function;
return true;
}
else return false;
}
//register a key and supplies the type of class which this key relates to
//this is then used by the default creater to create the required class
//when called
template <typename Type> bool register_key(Id key)
{
iter i;
if ( ( i = m_creater.find(key) ) == m_creater.end() )
{
m_creater[key] = &abstract_creater<Base,Type>;
return true;
}
else return false;
}
//remove a key and create function from the map
bool unregister_key(Id key)
{
iter i;
if ( ( i = m_creater.find(key) ) == m_creater.end() )
{
m_creater.erase(i);
return true;
}
else return false;
}

private:
std::map< Id , func > m_creater;
typedef typename std::map< Id, func >::iterator iter;

};

}


#endif//ABSTRCT_FACTORY_H_


Share this post


Link to post
Share on other sites
Ooh, this kind of code is unfortunately brittle. Sounds like an order-of-static-ctors issue: are you sure m_registry is initialized before insert is called? (use memory breakpoint)

BTW, (unrelated) hard-learned advice for any later readers pulling their hair out due to similar issues:

// IMPORTANT NOTE: if compiling this into a static lib and not using VC8's
// "use library dependency inputs" linking mode, the object file will be
// discarded because it does not contain any symbols that resolve another
// module's undefined external(s). for a discussion of this topic, see:
// http://forums.microsoft.com/MSDN/ShowPost.aspx?PostID=144087
// workaround: in the main EXE project, reference a symbol from this module,
// thus forcing it to be linked in. example:
// #pragma comment(linker, "/include:_wstartup_InitAndRegisterShutdown")
// (doing that in this module isn't sufficient, because it would only
// affect the librarian and its generation of the static lib that holds
// this file. instead, the process of linking the main EXE must be fixed.)

Share this post


Link to post
Share on other sites
Quote:
Original post by Timkin
Thanks guys... it was the omission of 'const' (I must be blind) that was causing the compilation problem...now I just need to work out what's causing the fatal error on startup on the call

EntityFactory::m_registry.insert(std::make_pair(_type,this));

reporting an access violation. 8( Ah well, at least that's one problem out of the way... thanks...

Timkin


EntityFactory::m_registry is probably getting constructed after ObjectFactory::register_this, and since the later's constructor calls insert() on the former.....boom! (construction order of globals and statics is defined within a translation unit, but not between them) If you make m_registry a local static of a function, the standard guarenttees that it's constructed the first time the function is called.


class EntityFactory
{
typedef std::map<OBJ_TYPE,EntityFactory*> FactoryMap;

static FactoryMap& getRegistry();
virtual BaseEntity* Create() = 0;

public:

EntityFactory(OBJ_TYPE type);
static BaseEntity* Construct(OBJ_TYPE type);
};

EntityFactory::FactoryMap& EntityFactory::getRegistry()
{
static FactoryMap registry;
return registry;
}

EntityFactory::EntityFactory(OBJ_TYPE type)
{
getRegistry().insert(std::make_pair(type,this));
}

BaseEntity* EntityFactory::Construct(OBJ_TYPE type)
{
FactoryMap::iterator it = getRegistry().find(type);
if (it != getRegistry().end())
{
EntityFactory* pObjectFactory = it->second;
return pObjectFactory->Create();
}
return 0;
}




Also cleaned it up a little. I couldn't see any reason for FactoryMap being public or m_registry being protected, so I made them both private. Also, Create() doesn't need to be accessible from the derived to be overriden by the derived, so I made it private too.

[Edited by - joanusdmentia on June 23, 2007 9:45:08 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Jan Wassenberg
Ooh, this kind of code is unfortunately brittle. Sounds like an order-of-static-ctors issue: are you sure m_registry is initialized before insert is called? (use memory breakpoint)


That's exactly the conclusion I've arrived at... my trace shows that 'register_this' is indeed the first thing being constructed... edit: I didnt see joanus' response before I finished mine..

It's working now, so thanks for the help guys... muchly appreciated... 8)

I think I'm going to consider a less "brittle" approach though during my next code revision... using static variables seems like it's going to be hard for other people to figure what the hell I've done.

Cheers,

Timkin

[Edited by - Timkin on June 23, 2007 9:46:44 AM]

Share this post


Link to post
Share on other sites
Quote:
I think I'm going to consider a less "brittle" approach though during my next code revision... using static variables seems like it's going to be hard for other people to figure what the hell I've done.

:)
Having slogged through all kinds of bugs and errors due to init order, I am faced with the fact that no method of automatic init has failed to cause problems.
- function tables generated at link-time (think .CRT$XI init section) are very prone to linker optimization / the problem mentioned above.
- singletons are difficult to init/shutdown repeatedly (which needs to be done by self-tests) and subject to shutdown order bugs.
- static-ctor registration can blow up as in the OP's case.
- on-demand if(!isInitialized) Init() calls within the module's externally accessible functions are easy to forget under maintenance.

So what do we conclude? A simple interface with explicit Init/Shutdown seems to be the safest way. This makes init order perfectly clear and there's no mysterious stuff going on behind your back that needs to be debugged.
These Init/Shutdown functions can handle being called more than once with a thread-safe and referenced-counted version of the !isInitialized check (see src/lib/module_init in the archive referenced here.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!