Segmentation fault when trying to remove element from a list

Question

In order to solve this problem I have, I wrote this little piece of code:

for (std::list<std::vector<int>>::iterator i = l.begin(); i != l.end(); ++i) {
  for (std::list<std::vector<int>>::iterator j = next(i, 1); j != l.end(); ++j) {
    if (includes((*j).begin(), (*j).end(), (*i).begin(), (*i).end())) {
      l.erase(j++);
    }
  }
}

The basic idea is, given an element of the list, to remove elements from the rest of the list that matches some criteria (in this case, a containment relation).

Executing this triggers a segmentation fault, which I can't understand. Can anyone give me a clue about this?

Don't mix `for`-increment with container mutation. – Kerrek SB Sep 24 '13 at 17:17 — Kerrek SB, Sep 24 '13 at 17:17

Pierre · Accepted Answer · 2013-09-24T20:30:44.967

0

The comment is right, thanks. The corrected code is:

for (std::list<colset>::iterator i = l.begin(); i != l.end(); ++i) {
  std::list<colset>::iterator j = next(i, 1);
  while (j != l.end()) {
    if (includes((*j).begin(), (*j).end(), (*i).begin(), (*i).end())) {
      j = l.erase(j);
    }
    else {
      ++j;
    }
  }
}

edited Sep 24 '13 at 20:30

answered Sep 24 '13 at 17:20

Pierre

6,084
5
32
52

2

you code was still not correct. I modified it. You can't use `l.erase(j++)`. You need to do `j = l.erase(j)` – Mark Lakata Sep 24 '13 at 17:24
@MarkLakata Thanks! However, the code as I first wrote it worked with my test case. The comments [here](http://stackoverflow.com/questions/596162/can-you-remove-elements-from-a-stdlist-while-iterating-through-it) were very helpful at understanding why I shouldn't do this. – Pierre Sep 24 '13 at 17:37
1

The problem with "Undefined Behavior" means that you can't predict if it is going to work or not :). Once you do `l.erase(j)` on `j`, then `j` points to junk, and `j++` is UB. – Mark Lakata Sep 24 '13 at 17:38
That's the postfix increment, which first increments j and then does an erase on the previous value. There is no UB involved. The code is actually rather idiomatic, at least for C++98, because there erase() returned void (at least for some containers). Using postfix increment in the else-branch is useless though, although harmless. – Ulrich Eckhardt Sep 24 '13 at 20:16
@UlrichEckhardt Thanks for the precision. As I do need to increment the vector in the `else` branch, I don't really get your point regarding this. Are you suggesting that `std::advance` would be more appropriate? – Pierre Sep 24 '13 at 20:21
No, advance() is not needed. However, in the else branch you can use `++j` or `j++`, but the former is potentially faster because it doesn't have to create a temporary for storing the previous value. As a general rule, use the prefix increment unless you need postfix increment. – Ulrich Eckhardt Sep 24 '13 at 20:26

Segmentation fault when trying to remove element from a list

1 Answers1