Multimap clears

Question

Here's a simple events system I've made using multimaps; When I use the CEvents::Add(..) method, it should insert and entry into the multimap. The thing is, when I trigger those events, the multimap appears to be empty. I'm sure I didn't call the delete method [CEvents::Remove]. Here's the code:

//Code:
..
CEvents Ev;
Ev.Add("onButtonBReleased",OutputFST);
..

// "CEvents.h"
class CEvents
{
public:

    void            Add                     ( string EventName, void(*fn)(void));   
    void            Remove                  ( string EventName, void(*fn)(void));
    void            Trigger                 ( string EventName );

//protected:

    bool            Found;

    std::multimap<string,void(*)(void)> EventsMap;
    std::multimap<string,void(*)(void)>::iterator EvMapIt;
};



//CEvents.cpp
void CEvents::Add (string EventName, void (*fn)(void))
{
if (!EventsMap.empty())
{
    Found = false;

    for (EvMapIt = EventsMap.begin(); EvMapIt != EventsMap.end(); EvMapIt++)
    {
        if ((EvMapIt->first == EventName) && (EvMapIt->second == fn))
        {
        CTools tools;
        tools.ErrorOut("Function already bound to same event... Not registering event");
                Found = true;
            } 
        }

        if (!Found)
        {
            EventsMap.insert(std::pair<string,void(*)(void)>(EventName,fn));
            std::cout<<"Added, with size "<<(int) EventsMap.size()<<std::endl; //Getting 1
        }
}
else
{
    EventsMap.insert (std::pair<string,void(*)(void)>(EventName,fn));
    std::cout<<"Added, with size "<<(int) EventsMap.size()<<std::endl; //Getting 1
}
}

void CEvents::Trigger (string EventName)
{
std::cout<<"Triggering init"<<std::endl;
std::cout<<(int) EventsMap.size()<<std::endl; //Getting 0

for (EvMapIt = EventsMap.begin(); EvMapIt != EventsMap.end(); EvMapIt++)
    {
        std::cout<<"Triggering proc"<<std::endl;
        if (EvMapIt->first == EventName)
    EvMapIt->second();
}
}

Answer 1

Ok, finally solved it; For future reference:

What happened is that every time an instance is declared, it redefines the multimap. The solution is to create a global pointer to the class.

Thanks to everyone who answered!

Answer 2

It's not supposed to be a code review site, but I can't help myself...

// "CEvents.h"
class CEvents
{
public:
    typedef void (*Callback)(void);

    // 1. Don't use `using namespace` in header files
    // 2. Pass by const reference to avoid a copy
    // 3. Function Pointers are easier to deal with when typedef'd
    void Add(std::string const& EventName, Callback fn);
    void Remove(std::string const& EventName, Callback fn);
    void Trigger(std::string const& EventName);

// Attributes should be `private` or `public`, `protected` is for functions.
// If you read otherwise, consider how this violates encapsulation.
//protected:

private: // cause nobody's touching my stuff lest they break it!

    // useless in this class, should be local variables in the routines
    // bool Found;
    // MapType::iterator EvMapIt;

    // typedef make life easier, spelling that out each time is just tiring.
    typedef std::multimap<std::string, Callback> MapType;
    MapType EventsMap;
};

Okay, so let's go for the source file.

//CEvents.cpp

// Whole rewrite to use idiomatic interfaces
void CEvents::Add(std::string const& EventName, Callback fn)
{
    // Retrieve the range of callbacks registered for "EventName"
    std::pair<MapType::iterator, MapType::iterator> const range =
        EventsMap.equal_range(EventName);

    // Check that this callback is not already registered.
    for (MapType::iterator it = range.first, end = range.second;
         it != end; ++it)
    {
        if (it->second == fn) {
            // Are you sure `ErrorOut` should not be a free function
            // or at least a `static` function ?
            // It feels weird instantiating this class.
            CTools tools;
            tools.ErrorOut("Function already bound to same event..."
                           " Not registering event");
            // If it is in there, nothing to do, so let's stop.
            return;
        }
    }

    // If we are here, then we need to add it.
    // Let's give a hint for insertion, while we are at it.
    EventsMap.insert(range.second, std::make_pair(EventName, fn));

    // the (int) cast was C-like (bah...) and unnecessary anyway
    std::cout << "Added, with size " << EventsMap.size() << std::endl; 
}


void CEvents::Trigger (std::string const& EventName)
{
    std::cout << "Triggering init" << std::endl;
    std::cout <<  EventsMap.size() << std::endl; //Getting 0

    // Retrieve the range of callbacks registered for `EventName`
    std::pair<MapType::const_iterator, MapType::const_terator> const range =
        EventsMap.equal_range(EventName);

    // Call each callback in turn
    for (MapType::const_iterator it = range.first, end = range.second;
         it != end; ++it)
    {
        it->second();
    }
}

Of course, it may not solve your issue, but it's so much shorter than it should help narrow it down.

Of course, it might be simpler to use a std::set<std::pair<std::string, Callback>> because it would ensure the unicity of (EventName, fn) pairs automatically... the code to dispatch the event would be slightly more complicated though, so not sure it would be a win (code wise or performance wise).

Answer 3

By the way, for make it faster, it is better to break your for, when you found the item:

for (EvMapIt = EventsMap.begin(); EvMapIt != EventsMap.end(); EvMapIt++)
{
    if ((EvMapIt->first == EventName) && (EvMapIt->second == fn))
    {
            CTools tools;
            tools.ErrorOut("Function already bound to same event... Not registering event");
            Found = true;
            break;
    } 

 }

Edit: "The insert member functions returns an iterator that points to the position where the new element was inserted into the multimap." (msdn)

to make sure you insert right, cout the return value of insert.

Answer 4

This is not how to use a map. You can find( key ) which will likely be faster than iterating over all the elements in your collection. If you want to ensure keys are unique then you can use an ordinary map rather than a multimap which expressly for the purpose of storing duplicate keys as unique entities.

EDIT: Updating based on your comment that keys shouldn't be unique. You should then use lower_bound and upper_bound for your search, this is better than checking all the keys as find (from memory) just returns the lower_bound. Alternatively you could just iterate the results of an equal range (As Mark suggested in the comments) which will result in the same outcome.

Just by glancing at your code (and by the lack of erase, clear or swap calls) I'd guess your problem is when you add your elements to the collection rather than them being 'emptied'.