1

I'm trying to erase the last element in the vector using iterator. But I'm getting segmentation fault when erasing the element.

Below is my code:

    for (vector<AccDetails>::iterator itr = accDetails.begin(); itr != accDetails.end(); ++itr) {
    if (username == itr->username) {
            itr = accDetails.erase(itr);
    }
}

Is there something wrong with my iteration?

Joel Seah
  • 674
  • 3
  • 13
  • 34
  • 2
    possible duplicate of [Remove elements of a vector inside the loop](http://stackoverflow.com/questions/8628951/remove-elements-of-a-vector-inside-the-loop) – Borgleader Aug 03 '13 at 04:28

2 Answers2

5

This is a good place to apply the remove/erase idiom:

accDetails.erase(
    std::remove_if(
        accDetails.begin(), accDetails.end(), 
        [username](AccDetails const &a) { return username == a.username; }),
     accDetails.end());

As a bonus, this is likely to be a little bit faster than what you were doing (or maybe quite a bit faster, if your vector is large). Erasing each item individually ends up as O(N2), but this will be O(N), which can be pretty significant when/if N gets large.

If you can't use C++11, the lambda won't work, so you'll need to encode that comparison separately:

class by_username { 
    std::string u;
public:
    by_username(std::string const &u) : u(u) {}
    bool operator()(AccDetails const &a) { 
        return u == a.username;
    }
};

accDetails.erase(
    std::remove_if(accDetails.begin(), accDetails.end(), by_username(username)), 
    accDetails.end());

Alternatively, you can overload operator== for your AccDetails class, and handle the comparison there. For example:

#include <vector>
#include <iostream>
#include <algorithm>
#include <string>
#include <iterator>

class AccDetail {
    std::string name;
    int other_stuff;
public:
    AccDetail(std::string const &a, int b) : name(a), other_stuff(b) {}

    bool operator==(std::string const &b) {
        return name == b;
    }

    friend std::ostream &operator<<(std::ostream &os, AccDetail const &a) {
        return os << a.name << ", " << a.other_stuff;
    }
};

int main(){
    std::vector<AccDetail> ad = { {"Jerry", 1}, { "Joe", 2 }, { "Bill", 3 } };

    std::cout << "Before Erase:\n";
    std::copy(ad.begin(), ad.end(), std::ostream_iterator<AccDetail>(std::cout, "\n"));
    ad.erase(
        std::remove(ad.begin(), ad.end(), "Joe"),
        ad.end());

    std::cout << "\nAfter Erasing Joe:\n";
    std::copy(ad.begin(), ad.end(), std::ostream_iterator<AccDetail>(std::cout, "\n"));
}
Jerry Coffin
  • 476,176
  • 80
  • 629
  • 1,111
  • I'm getting *expected primary expression* error at this line `[username]...`. So do i need to write a operator() in my struct? – Joel Seah Aug 03 '13 at 04:59
-2

I learn a safe way to erase elements from my leader. Firstly, find all the elements. Secondly, erase them one by one.

queue< vector<AccDetails>::iterator > q;
for (vector<AccDetails>::iterator itr = accDetails.begin(); itr != accDetails.end(); ++itr) {
    if (username == itr->username) {
        //itr = accDetails.erase(itr);
        q.push(itr);
    }
}
while(!q.empty()){
    vector<AccDetails>::iterator itr = q.front();
    accDetails.erase(itr);
    q.pop();
}
alexis
  • 569
  • 5
  • 12
  • @DanielKO, you can not just copy the code and run. I just show the method, a method to deal with such kind of problem – alexis Aug 03 '13 at 07:27
  • The algorithm is wrong. It's not only a quadratic time algorithm, all elements erased after the first one are wrong. It might even access outside of the sequence. Try using it to erase all even numbers in the vector {1, 2, 3, 4, 5, 6}. – DanielKO Aug 05 '13 at 06:38
  • @DanielKO, yes, you are right. I mistake the usage. What the iterator points will change. Then it will cause the error. Something like map with key and value can use my algorithm. If we need to earse something base on value. Use the iterator to find the keys, then remove the elements by keys – alexis Aug 08 '13 at 02:50