views:

167

answers:

6

When implementing a MessageFactory class to instatiate Message objects I used something like:

class MessageFactory 
{
  public:
    static Message *create(int type)
    {
       switch(type) {
         case PING_MSG:
            return new PingMessage();
         case PONG_MSG:
            return new PongMessage();
         ....
    }
}

This works ok but every time I add a new message I have to add a new XXX_MSG and modify the switch statement.

After some research I found a way to dynamically update the MessageFactory at compile time so I can add as many messages as I want without need to modify the MessageFactory itself. This allows for cleaner and easier to maintain code as I do not need to modify three different places to add/remove message classes:

#include <stdio.h>                                                                                                                                                                           
#include <stdlib.h>                                                                                                                                                                          
#include <string.h>                                                                                                                                                                          
#include <inttypes.h>                                                                                                                                                                        

class Message                                                                                                                                                                                
{                                                                                                                                                                                            
   protected:                                                                                                                                                                                
      inline Message() {};                                                                                                                                                                   

   public:                                                                                                                                                                                   
      inline virtual ~Message() { }                                                                                                                                                          
      inline int getMessageType() const { return m_type; }                                                                                                                                   
      virtual void say() = 0;                                                                                                                                                                

   protected:                                                                                                                                                                                
      uint16_t m_type;                                                                                                                                                                       
};                                                                                                                                                                                           

template<int TYPE, typename IMPL>                                                                                                                                                            
class MessageTmpl: public Message                                                                                                                                                            
{                                                                                                                                                                                            
   enum { _MESSAGE_ID = TYPE };                                                                                                                                                              
   public:                                                                                                                                                                                   
      static Message* Create() { return new IMPL(); }                                                                                                                                        
      static const uint16_t MESSAGE_ID; // for registration                                                                                                                                  

   protected:                                                                                                                                                                                
      MessageTmpl() { m_type = MESSAGE_ID; } //use parameter to instanciate template                                                                                                         
};                                                                                                                                                                                           

typedef Message* (*t_pfFactory)();                                                                                                                                                           
class MessageFactory⋅                                                                                                                                                                        
{                                                                                                                                                                                            
   public:                                                                                                                                                                                   
     static uint16_t Register(uint16_t msgid, t_pfFactory factoryMethod)                                                                                                                     
     {                                                                                                                                                                                       
       printf("Registering constructor for msg id %d\n", msgid);                                                                                                                             
       m_List[msgid] = factoryMethod;                                                                                                                                                        
       return msgid;                                                                                                                                                                         
     }                                                                                                                                                                                       

     static Message *Create(uint16_t msgid)                                                                                                                                                  
     {                                                                                                                                                                                       
       return m_List[msgid]();                                                                                                                                                               
     }                                                                                                                                                                                       
     static t_pfFactory m_List[65536];                                                                                                                                                       
};  

template <int TYPE, typename IMPL>                                                                                                                                                           
const uint16_t MessageTmpl<TYPE, IMPL >::MESSAGE_ID = MessageFactory::Register(                                                                                                              
     MessageTmpl<TYPE, IMPL >::_MESSAGE_ID, &MessageTmpl<TYPE, IMPL >::Create);                                                                                                              

class PingMessage: public MessageTmpl < 10, PingMessage >                                                                                                                                    
{⋅                                                                                                                                                                                           
  public:                                                                                                                                                                                    
  PingMessage() {}                                                                                                                                                                           
  virtual void say() { printf("Ping\n"); }                                                                                                                                                   
};                                                                                                                                                                                           

class PongMessage: public MessageTmpl < 11, PongMessage >                                                                                                                                    
{⋅                                                                                                                                                                                           
  public:                                                                                                                                                                                    
  PongMessage() {}                                                                                                                                                                           
  virtual void say() { printf("Pong\n"); }                                                                                                                                                   
};                                                                                                                                                                                           

t_pfFactory MessageFactory::m_List[65536];                                                                                                                                                   

int main(int argc, char **argv)                                                                                                                                                              
{                                                                                                                                                                                            
  Message *msg1;                                                                                                                                                                             
  Message *msg2;                                                                                                                                                                             

  msg1 = MessageFactory::Create(10);                                                                                                                                                         
  msg1->say();                                                                                                                                                                               

  msg2 = MessageFactory::Create(11);                                                                                                                                                         
  msg2->say();                                                                                                                                                                               

  delete msg1;                                                                                                                                                                               
  delete msg2;                                                                                                                                                                               

  return 0;                                                                                                                                                                                  
} 

The template here does the magic by registering into the MessageFactory class, all new Message classes (e.g. PingMessage and PongMessage) that subclass from MessageTmpl.

This works great and simplifies code maintenance but I still have some questions about this technique:

  1. Is this a known technique/pattern? what is the name? I want to search more info about it.

  2. I want to make the array for storing new constructors MessageFactory::m_List[65536] a std::map but doing so causes the program to segfault even before reaching main(). Creating an array of 65536 elements is overkill but I have not found a way to make this a dynamic container.

  3. For all message classes that are subclasses of MessageTmpl I have to implement the constructor. If not it won't register in the MessageFactory.

    For example commenting the constructor of the PongMessage:

     class PongMessage: public MessageTmpl < 11, PongMessage >       
     {                                                                                                                                                                                           
       public:                                                                                                                                                                                    
        //PongMessage() {} /* HERE */                                                                                                                                                                          
        virtual void say() { printf("Pong\n"); }                   
     };
    

    would result in the PongMessage class not being registered by the MessageFactory and the program would segfault in the MessageFactory::Create(11) line. The question is
    why the class won't register? Having to add the empty implementation of the 100+ messages I need feels inefficient and unnecessary.

A: 

2: you could use a dynamic container, but then you'd also had to change how the registration etc. For example, you could use a map with an int as key and a function pointer as element:

typedef Message* ( *NewMessageFun )();

template< class tMessage >
Message* NewMessage()
{
  return new tMessage();
};

class PingMessage : public MessageImpl
{
public:
  enum{ _MESSAGE_ID = 10 };
};

class PongMessage
{
public:
  enum{ _MESSAGE_ID = 11 };
}

//factory
std::map< int, NewMessageFun > mymap;
bool Register( const int type, NewMessageFun fun )
{
  if( mymap.contains( type ) )
    return false; //already registered!
  mymap[ type ] = fun;
  return true;
}

template< class tMessage >
bool RegisterAny() //shortcut
{
  return Register( tMessage::_MESSAGE_ID, NewMessage< tMessage > );
}
//

//main
factory.RegisterAny< PingMessage >();
factory.RegisterAny< PongMessage >();
//

Or, in your current code, just use a sensible allocation size and have runtime bounds checking to see there are too much registrations. And maybe supply an 'Unregister' method.

stijn
Yes I tried a map but it segfaulted... but thanks to Josh Kelley and Noah Roberts answers I found the problem (static initialization order). Guess I will have to go Singleton here but I heard these are evil.
Horacio
singletons are not always evil. If you think, for your case, a singleton is the best approach, then it is.
stijn
+3  A: 

Answer One

The general technique of deriving a class like this is the Curiously Recurring Template Pattern (CRTP):

class PingMessage: public MessageTmpl < 10, PingMessage > 

Your specific technique of using a template class's static member initialization to register subclasses of that class is (IMO) simply brilliant, and I've never seen that before. A more common approach, used by unit test frameworks like UnitTest++ and Google Test, is to provide macros that declare both a class and a separate static variable initializing that class.

Answer Two

Static variables are initialized in the order listed. If you move your m_List declaration before your MessageFactory::Register calls, you should be safe. Also keep in mind that if you start declaring Message subclasses in more than one file, you'll have to wrap m_List as a singleton and check that it's initialized before each use, due to the C++ static initialization order fiasco.

Answer Three

C++ compilers will only instantiate template members that are actually used. Static members of template classes is not an area of C++ that I've used much, so I could be wrong here, but it looks like providing the constructor is enough to make the compiler think that MESSAGE_ID is used (thus ensuring that MessageFactory::Register is called).

This seems very unintuitive to me, so it may be a compiler bug. (I was testing this in g++ 4.3.2; I'm curious to know how Comeau C++, for example, handles it.)

Explicitly instantiating MESSAGE_ID also suffices, at least in g++ 4.3.2:

template const uint16_t PingMessage::MESSAGE_ID;

But that's even more unnecessary work than providing an empty default constructor.

I can't think of a good solution using your current approach; I'd personally be tempted to switch to a technique (such as macros or using a script to generate part of your source files) that relied less on advanced C++. (A script would have the added advantage of easing maintenance of MESSAGE_IDs.)

In response to your comments:

Singletons are generally to be avoided because they're often overused as poorly disguised global variables. There are a few times, however, when you really do need a global variable, and a global registry of available Message subclasses is one of those times.

Yes, the code that you provided is initializing MESSAGE_ID, but I was talking about explicitly instantiating each subclass's instance of MESSAGE_ID. Explicit instantiation refers to instructing the compiler to instantiate a template even if it thinks that that template instance won't otherwise be used.

I suspect that the static function with the volatile assignment is there to trick or force the compiler into generating the MESSAGE_ID assignment (to get around the problems that dash-tom-bang and I pointed out with the compiler or linker dropping or not instantiating the assignment).

Josh Kelley
The OP should also keep in mind that many linkers strip unreferenced objects automatically, even if they do interesting work (that is, the static members' initialization triggering registration into a master list). Visual Studio, for instance, will happily throw away static member initialization in libraries (that is, code that is not part of your "executable" project) if the executable doesn't explicitly reference that static object in some way. You can add each one to a list of "implicitly referenced objects" in VS, but that's even more of a hassle IMO.
dash-tom-bang
Thanks for your answers. I learned a lot from them.I take no credit from this code as I copied it from a japanese networking library called nine.I am looking now into the singleton thing but I understand these are to be avoided.I though the template that registers the constructors is also explicitly initializing the MESSAGE_ID? you can see an equal sign there.Finally the original implementation has this code in the MessageTmpl template: static void Enable() { volatile uint16_t x = MESSAGE_ID; }But I have no idea how this is used to enable the constructor registration.
Horacio
+2  A: 
Noah Roberts
A: 

here is slightly modified listing using map

#include <map>
#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <inttypes.h>

//typedef Message *;
class MessageFactory {
 public:
    struct register_base {
        virtual int id() const = 0;
        virtual Message* new_() = 0;
    };
    template<class C>
    struct register_ : register_base {
        static const int ID;
        register_() : id_(ID)  {} // force ID initialization
        int id() const { return C::factory_key; }
        Message* new_() { return new C(); }
    private:
        const int id_;
    };
    static uint16_t Register(register_base* message) {
        printf("Registering constructor for msg id %d\n", message->id());
        m_List[message->id()] = message;
        return message->id();
    }
    static Message *Create(uint16_t msgid) {
        return m_List[msgid]->new_();
    }
    static std::map<int,register_base*> m_List;
};
std::map<int, MessageFactory::register_base*> MessageFactory::m_List;

template<class C>
const int MessageFactory::register_<C>::ID =
    MessageFactory::Register(new MessageFactory::register_<C>());


class Message {
public:
    virtual ~Message() {}
    int getMessageType() const {
        return m_type;
    }
    virtual void say() = 0;
protected:
    uint16_t m_type;
};

class PingMessage: public Message, private MessageFactory::register_<PingMessage> {
public:
    static const int factory_key = 10;
    PingMessage() { } // must call explicitly to register
    virtual void say() {
        printf("Ping\n");
    }
};

class PongMessage:public Message, private MessageFactory::register_<PongMessage> {
public:
    static const int factory_key = 11;
    PongMessage() { }
    void say() {
        printf("Pong\n");
        std::cout   << this->id() << std::endl;
    }
};



int main(int argc, char **argv)
{
    Message *msg1;
    Message *msg2;

    msg1 = MessageFactory::Create(10);
    msg1->say();

    msg2 = MessageFactory::Create(11);
    msg2->say();

    delete msg1;
}
aaa
Thanks, I fixed this problem by moving the std::map declaration before the MessageTmpl registration code as mentioned by Josh Kelley. Now I can use a std::map instead of a fixed array.Still I need to make this a Singleton as to avoid problems as the message number increases.
Horacio
A: 

This is a modified version that uses a MessageFactory singleton and a std::map to store constructors. It works great so far but comments are welcome.

I am still trying to find a way to avoid creating constructors for each message class. I know is possible because the original library can do it. Unfortunately I only have the header files so no idea on the implementation details.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <inttypes.h>
#include <map>

class Message
{
   protected:
      inline Message() {};

   public:
      inline virtual ~Message() { }
      inline int getMessageType() const { return m_type; }
      virtual void say() = 0;

   protected:
      uint16_t m_type;
};

template<int TYPE, typename IMPL>
class MessageTmpl: public Message
{
   enum { _MESSAGE_ID = TYPE };
   public:
     static Message* Create() { return new IMPL(); }
     static const uint16_t MESSAGE_ID; // for registration
     static void Enable() { volatile uint16_t x = MESSAGE_ID; }
   protected:
      MessageTmpl() { m_type = MESSAGE_ID; } //use parameter to instanciate template
};

class MessageFactory 
{
   public:
     typedef Message* (*t_pfFactory)();

     static MessageFactory *getInstance()
     {
       static MessageFactory fact;
       return &fact;
     }

     uint16_t Register(uint16_t msgid, t_pfFactory factoryMethod)
     {
       printf("Registering constructor for msg id %d\n", msgid);
       m_List[msgid] = factoryMethod;
       return msgid;
     }

     Message *Create(uint16_t msgid)
     {
       return m_List[msgid]();
     }

     std::map<uint16_t, t_pfFactory> m_List;

  private:
     MessageFactory() {};
     MessageFactory(MessageFactory const&) {};
     MessageFactory& operator=(MessageFactory const&);
     ~MessageFactory() {};
};

//std::map<uint16_t, t_pfFactory> MessageFactory::m_List;

template <int TYPE, typename IMPL>
const uint16_t MessageTmpl<TYPE, IMPL>::MESSAGE_ID = MessageFactory::getInstance()->Register(
     MessageTmpl<TYPE, IMPL >::_MESSAGE_ID, &MessageTmpl<TYPE, IMPL >::Create);


class PingMessage: public MessageTmpl < 10, PingMessage >
{ 
  public:
  PingMessage() {}
  virtual void say() { printf("Ping\n"); }
};

class PongMessage: public MessageTmpl < 11, PongMessage >
{ 
  public:
  PongMessage() {}
  virtual void say() { printf("Pong\n"); }
};

int main(int argc, char **argv)
{
  Message *msg1;
  Message *msg2;

  msg1 = MessageFactory::getInstance()->Create(10);
  msg1->say();

  msg2 = MessageFactory::getInstance()->Create(11);
  msg2->say();

  delete msg1;
  delete msg2;

  return 0;
}
Horacio
A: 

For all message classes that are subclasses of MessageTmpl I have to implement the constructor. If not it won't register in the MessageFactory.

I was experimenting with this idea and came up with a way to force instantiation of the registration variable without having to make anything in the derived class. Make a virtual function in the template that accesses the registration variable. This forces the function to be instantiated because the virtual function must be there whether or not it is ever called.

Here's my scratch code:

#include <boost/array.hpp>
#include <iostream>

struct thingy
{
  virtual ~thingy() {}
  virtual void print_msg() const = 0;
  virtual size_t id() const = 0;

  bool registered_already() const { return registered; }
protected:
  bool registered;
};

struct holder
{
  enum index {
    ID_OPEN
  , ID_SAVE
  , ID_SAVEAS
  , COUNT
  };

  static holder& instance()
  {
    static holder inst;
    return inst;
  }

  thingy& operator[] (size_t i)
  {
    assert(thingys[i] && "Not registered.");
    return *thingys[i];
  }

  bool registered(size_t i) const { return thingys[i] != 0; }

  ~holder() { std::for_each(thingys.begin(), thingys.end(), [](thingy* t) { delete t; }); }

  index reg(thingy* t, index i)
  {
    assert( !thingys[i] && "Thingy registered at this ID already" );
    thingys[i] = t;
    return i;
  }

private:

  holder() : thingys() {}

  boost::array< thingy*, COUNT > thingys;
};

template < typename Derived, holder::index i >
struct registered_thingy : thingy
{
  size_t id() const { return registration; }
private:
  static holder::index registration;
};

template < typename T, holder::index i >
holder::index registered_thingy<T,i>::registration = holder::instance().reg(new T, i);

struct thingy1 : registered_thingy<thingy1,holder::ID_OPEN>
{
  void print_msg() const { std::cout << "thingy1\n"; }
};

struct thingy2 : registered_thingy<thingy2, holder::ID_SAVE>
{
  void print_msg() const { std::cout << "thingy2\n"; }
};

struct thingy3 : registered_thingy<thingy3, holder::ID_SAVEAS>
{
  void print_msg() const { std::cout << "thingy3\n"; }
};



int main()
{
  holder::instance()[holder::ID_OPEN].print_msg();

  std::cin.get();
}
Noah Roberts
Sorry to say I cannot make this code work. I get the Not registered assertion which means the thingys are not being registered. Again if I provide the thingys constructors the code works ok.
Horacio
What compiler are you using? It works for me and as far as I can tell it should. I believe the compiler is forced to instantiate id(), which uses the variable...
Noah Roberts
Damn. 14.7.1/9: "... It is unspecified whether or not an implementation implicitly instantiates a virtual member function of a class template if the virtual member function would not otherwise be instantiated."
Noah Roberts
You might try to add a call to id() in main. That should force the issue.
Noah Roberts