CODEWITHSUNDEEP
c++vector
Sumsar1812
Sumsar1812

Reputation: 618

Vector iterator erase giving me a runtime error?

So I got a methode inside my class, and what this class is supposed to do is, check if the vector i have in the .h file have values bewtween double low & double high and then delete those and at last return how many "spaces" got removed

So I tried a few things and i always get runtime errors, it seems to be in the for loop, but i can´t figure out why.

Here is what i tried,

First I tried to just do it the way I felt like it would work:

int datastorage::eraseDataPointsBetween(double low,double high) 
{
    int antal = 0;
    for (vector<double>::iterator i = data_.begin(); i !=data_.end();i++)
    {
        if (*i >=low && *i <=high)
        {
            data_.erase(i);
            antal++;
        }

    }
    return antal;
}

But then I tried to do some debugging and I could see that it actually doesn´t make sence to have it like that as when something gets deleted it still gets incremented(so if we delete "space 2" it would actually check space 4 next time(as spot 3 get to be spot 2 after erase)))

So I tried to change it to this

int datastorage::eraseDataPointsBetween(double low,double high) 
{
    int antal = 0;
    for (vector<double>::iterator i = data_.begin(); i !=data_.end();)
    {
        if (*i >=low && *i <=high)
        {
            data_.erase(i);
            antal++;
        }
        else 
            i++;
    }
    return antal;
}

Where it only increment the i whenever i do not remove a space(so if I delete "space 2", it will check the new "space 2" next run)

This also gives me a syntax error expression: vector iterators incompatible

Hope you can help because I'm pretty lost

Upvotes: 1

Views: 1365

Answers (2)

4pie0
4pie0

Reputation: 29724

vector::erase invalidates iterator so you cannot use it after call to erase. You should erase from a vector this way:

int datastorage::eraseDataPointsBetween( double low, double high)  {
  int antal = 0;
  for( vector<double>::iterator i = data_.begin(); i !=data_.end())
  {  
     if( (*i >= low) && (*i <= high))
     {
        i = data_.erase( i); // new (valid) iterator is returned
        ++antal;
     }
     else
     {
        ++i;
     }

   return antal;
}

Upvotes: 5

PaulMcKenzie
PaulMcKenzie

Reputation: 35440

You should use the remove_if() and erase. The reason why this is somewhat more stable than writing your own loops is simple -- you can't get into trouble using invalid iterators.

#include <algorithm>
#include <vector>

struct IsBetween
{
   double low, high;
   IsBetween(double l, double h) : low(l), high(h) {}
   bool operator()(double d) const { return d >= low && d <= high; }
};

void datastorage::eraseDataPointsBetween(double low,double high) 
{
   std::vector<double>::iterator it = std::remove_if(data.begin(), data.end(), IsBetween(low, high));
   data.erase(it, data.end());
}

There are no loops, and note the use of remove_if() with a function object IsBetween.

The bottom line is that you should minimize the attempt to write loops that erase items in a container while you're looping over the container. There is remove_if(), remove(), partition(), etc. that moves the data you want to focus on to one end of the container.

As much as you'll see answers that erase an iterator while looping, and it seems safe, how many will remember the rules of what is returned, or rather simply, write the loop incorrectly? (even experienced C++ programmers could have written the loop incorrectly on the first cut). So use the algorithms to do this work for you.

For remove_if(), the "bad" data is moved to the end, where you can now easily erase them.

Upvotes: 3

Related Questions