RunTime Error : map/set iterators incompatible

Question

I have a runtime error "map/set iterators incompatible" at line 8.

void Manager::Simulate(Military* military, Shalishut* shalishut,char* args[]){
    Simulation* simulation = Simulation::GetInstance();
    Time* time = Time::GetInstance();

    multimap<int,Task*>::iterator itTasks;
    itTasks = simulation->GetTasks().begin();
    while(itTasks != simulation->GetTasks().end()){
      while (itTasks->second->GetTimeStamp() == time->GetTime()){ /*line 8 - ERROR*/
            TaskExecute(itTasks->second,military,shalishut,args);
            itTasks++;
        }
        // Unit take car of vehicles
        time->TimeIncrease();
    }

}

Simulation is declared as a multimap<int,Task*>. What is the problem?

Answer 1

I'm going to take a wild guess and say that the Simulation::GetTasks() signature looks like this:

multimap<int,Task*> GetTasks() const;

This creates a new multimap (a copy) each time you call it.

When comparing iterators, both of the multimap<int,Task*> iterators must come from the same container; since you're getting a new copy each time you call GetTasks(), you violate this constraint, and this is the source of your error. You also have another problem - the temporary multimap copies are destroyed after the statement they're created in, so your iterators are invalidated instantly.

You have two choices; one is to capture a copy locally and use that copy consistently:

multimap<int,Task*> tasks = simulation->GetTasks();
multimap<int,Task*>::iterator itTasks;
itTasks = tasks.begin();
while(itTasks != tasks.end()){
  while (itTasks->second->GetTimeStamp() == time->GetTime()){
        TaskExecute(itTasks->second,military,shalishut,args);
        itTasks++;
    }
    // Unit take car of vehicles
    time->TimeIncrease();
}

Another is to have GetTasks() return a reference to a persistent multimap, ensuring the same one is used each time:

multimap<int,Task*> &GetTasks();

Or a const reference:

const multimap<int,Task*> &GetTasks() const;

This has the advantage of avoiding the (potentially large) overhead of copying the multimap.

Note that using a const reference requires using const_iterators to step through the multimap. I would recommend defining both const and non-const accessors (C++ will pick the right one based on if the Simulation pointer or reference is const), unless you want to disallow direct modification of the underlying multimap entirely, in which case you can define only the const variant.