0

I have two vectors of the same type, shown below:

std::vector<Task*> ToRun;
std::vector<Task*> Completed;

When a Task is finished, it needs to be automatically moved from the ToRun vector container into the Completed vector.

ToRun Vector contains TaskOne TaskTwo TaskThree ... TaskN

Completed Vector contains nothing

After the first loop through the ToRun vector, both vectors should look like below:

ToRun vector contains TaskTwo TaskThree ... TaskN

Completed Vector contains TaskOne

The second loop through the ToRun vector should look like below:

ToRun vector contains TaskThree ... TaskN

Completed Vector contains TaskOne TaskTwo

I have the below code but I'm receiving the following error:

Vector Moving Error

Code snippet:

for (auto iter = ToRun.begin(); iter != ToRun.end(); iter++)
{
    (*iter)->dump(os);

    Completed.push_back(std::move(ToRun.front()));
    ToRun.erase(ToRun.begin());
}

I've tried looking at this and this SO answer however I'm having no luck. From what is happening, I think that the iterator points to null after I've moved the front element.

Should I attempt to move the Task from one container to another and then remove instead of erase?

Community
  • 1
  • 1
Sean
  • 507
  • 1
  • 9
  • 27
  • Why the `erase`? I.e., what is `ToRun` used for **other** than this? – Cheers and hth. - Alf Nov 24 '16 at 00:01
  • @Cheersandhth.-Alf `ToRun` is simply a vector of specific Tasks to execute. The number of tasks depends on a configuration file so there may be a lot, or a little. I'm very inexperienced with STL containers and iterators so I've been exploring my options – Sean Nov 24 '16 at 00:10
  • Well, then there's no need to `erase` each item one at a time (which is quadratic time in the order you're doing it). Simply clear the vector at the end. Or do nothing. – Cheers and hth. - Alf Nov 24 '16 at 00:32

2 Answers2

1

After this statement

ToRun.erase(ToRun.begin());

the iterator iter is invalid.

From the C++ Standard's description of the method erase

3 Effects: Invalidates iterators and references at or after the point of the erase.

Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
1

Erasing while iterating is always messy. If you erase the current element, how can ++ work?

Instead try

while (!ToRun.empty()) // loop until empty
{
    ToRun.front()->dump(os);

    Completed.push_back(std::move(ToRun.front()));
    ToRun.erase(ToRun.begin());
}

or something similar

But that's gross when you stop and think about it. Every erase shifts the rest of the elements in ToRun back one slot adding a large amount of unnecessary iterating and copying.

for (auto & run: ToRun) // or old school iterator if you prefer
{
    run->dump(os);
    Completed.push_back(std::move(ToRun.front()));
}
toRun.clear();

clear executes only once and obliterates the whole container in one shot. Much cleaner. Just one iteration to make sure the destructors are called, and you don't destroy pointers. Winning!

will have the same effect (assuming this isn't multithreaded, and if it is you have serious concurrency issues).

Cheersandhth.-Alf also brings up the point that since ToRun appears to contain pointers, std::move is not necessary. All that is being moved is a pointer, and that's trivial effort.

user4581301
  • 33,082
  • 7
  • 33
  • 54