c++ vector erase function not working for specific words?

Question

I am using a very simple function in c++, vector.erase(), here's what I have (I'm trying to erase all instances of these three keywords from a .txt file):

First I use it in two separate for loops to erase all instances of <event> and </event>, this works perfectly and outputs the edited text file with no more instances of those words.

for (int j = 0; j< N-counter; j++) {

    if(myvec[j] == "<event>") {
        myvec.erase(myvec.begin()+j);
    }
}

for (int j = 0; j< N-counter; j++) {

    if(myvec[j] == "</event>") {
        myvec.erase(myvec.begin()+j);
    }
}

However, when I add a third for loop to do the EXACT same thing, literally just copy and paste with a new keyword as follows:

for (int j = 0; j< N-counter; j++) {

    if(myvec[j] == "</LesHouchesEvents>") {
        myvec.erase(myvec.begin()+j);
    }
 }

It compiles and executes, however it completely destroys the .txt file, making it completely un-openable, and when i cat it, I just get a bunch of crazy symbols.

I have tried switching the order of these for loops, even getting rid of the first two for loops entirely, everything I can think of, alas it just will not work for the keyword </LesHouchesEvents> for some strange reason.

Hello... please see the guidelines for producing a [Minimal, Complete, Verifiable Example](http://stackoverflow.com/help/mcve). I would also suggest reading through [Simple, Self-Contained, Correct (Compilable) Example](http://sscce.org/). You are speaking specifically about a problem with an output file, yet there is no file I/O shown here. *(Additionally: doing iteration in this way is not how you're intended to traverse standard library collections; `myvec.begin() + j` may be costly for some collection types that are not implemented as arrays under the hood.)* — HostileFork says dont trust SE, Jun 05 '15 at 00:24
Off topic, but worth fixing. Removing an element from the vector without updating the for loop's exit condition is going to make you overrun the end of the vector. Incrementing the index after removing an element means you will skip an element. Look up the erase-remove idiom for solutions to these problems. — user4581301, Jun 05 '15 at 00:33
possible duplicate of [Remove elements of a vector inside the loop](http://stackoverflow.com/questions/8628951/remove-elements-of-a-vector-inside-the-loop) — Ami Tavory, Jun 05 '15 at 00:45

Remy Lebeau · Accepted Answer · 2015-06-05T00:41:13.350

Your loops are not taking into account that when you erase() an element from a vector, the indexes of the remaining elements will decrement accordingly. So your loops will eventually exceed the bounds of the vector once you have erased at least 1 element. You need to take that into account:

std:string word = ...;
size_t count = N-counter;

for (int j = 0; j < count;) {
    if(myvec[j] == word) {
        myvec.erase(myvec.begin()+j);
        --count;
    }
    else {
        ++j;
    }
}

With that said, it would be safer to use iterators instead of indexes. erase() returns an iterator to the element that immediately follows the removed element. You can use std::find() for the actual searching:

#include <algorithm>

std::vector<std::string>::iterator iter = std::find(myvec.begin(), myvec.end(), word);
while (iter != myvec.end())
{
    iter = myvec.erase(iter);
    iter = std::find(iter, myvec.end(), word);
}

Or, you could just use std::remove() instead:

#include <algorithm>

myvec.erase(std::remove(myvec.begin(), myvec.end(), word), myvec.end());

Thank you! I got it to work by using myvec.size() as the loop bounds, and it worked great. — khfrekek, Jun 05 '15 at 16:25

score 1 · Answer 2 · answered Jun 05 '15 at 00:32

I don't know if this is your specific problem or not, but this loop is almost surely not what you want.

Note the documentation for erase - it "shifts" left the remaining elements. Unfortunately, your code still increments j, meaning you're skipping the next element:

for (int j = 0; j< N-counter; j++) { // <- Don't increment j here

    ...
    myvec.erase(myvec.begin()+j); // <- increment it only if this didn't happen.

}

You'll also need to adjust your loop's halting condition.

score 0 · Answer 3 · answered Jun 05 '15 at 00:36

0

Even assuming you got it working, this is nearly the worst possible way to remove items from a vector.

You almost certainly want the remove/erase idiom here, and you probably want to do all the comparisons in a single pass, so it's something like this:

std::vector<std::string> bad = { 
    "<event>", 
    "</event>", 
    "</LesHouchesEvents>"
};

myvec.erase(std::remove_if(my_vec.begin(), my_vec.end(),
    [&](std::string const &s) {
        return std::find(bad.begin(), bad.end(), s) != bad.end(); 
    }), 
    my_vec.end());

answered Jun 05 '15 at 00:36

Jerry Coffin

476,176
80
629
1,111

Note that this example is using a lambda, which requires a C++11 compiler. You can use a standalone function, or a record with an overridden `operator()`, if you want to use `remove_if()` in an earlier compiler. But if you are going to use C++11, consider using `std::array` instead of `std::vector` for your lookup list (if the list is known at compile-time). – Remy Lebeau Jun 05 '15 at 00:39

c++ vector erase function not working for specific words?

3 Answers3