1
for (Shape *i : shapes) {
    for (Shape *j : shapes) {
        if (i != j) {
            if (check(i,j)){
                shapes.erase(remove(shapes.begin(), shapes.end(), i), shapes.end());

this causes an error because it's going to carry on iterating even though i does not exist, my question is how do I cleanly do this? currently I get an error "vector iterator not incrementable"

Can i just exit the second loop and continue in the first one?

user1234
  • 13
  • 4
  • 2
    related/dupe: http://stackoverflow.com/questions/8597240/how-to-delete-an-element-from-a-vector-while-looping-over-it – NathanOliver Dec 12 '16 at 21:01
  • [range-based-for + sequence modification = bad idea](http://stackoverflow.com/questions/20317068/can-we-erase-the-items-in-range-based-for-loop-in-c11) – WhozCraig Dec 12 '16 at 21:02

3 Answers3

2

You cannot erase elements from a vector when you are iterating it by for range loop, as internally it uses iterators that would be invalidated. This should work:

auto end = shapes.end();
for( auto it = shapes.begin(); it != end; ++it ) {
    end = shapes.erase( std::remove_if( std::next( it ), shapes.end(), [it]( Shape *s ) { 
             return check( *it, s ); 
         }, shapes.end() );
}

Note this code is slightly more effective than yours but it assumes that check( s1, s2 ) == check( s2, s1 ), if you can change your check() function to do strict ordering comparison, rather than equivalence, then you would be able to use std::sort and std::unique which are even more effective.

Slava
  • 43,454
  • 1
  • 47
  • 90
1

You cannot use a range-for loop in this case. Instead use a standard loop with iterators:

for (auto iter = shapes.begin(); iter != shapes.end(); iter++)

Mikel F
  • 3,567
  • 1
  • 21
  • 33
1

You can't modify the positioning of your shapes elements while using ranged-based for loops. The range loop uses iterators internally, and erasing vector elements invalidates existing iterators.

Try something more like this instead:

auto iter = shapes.begin();
auto end = shapes.end();
while (iter != end) {
    auto iter2 = shapes.begin();
    bool erased = false;
    while (iter2 != end) {
        if ((iter != iter2) && check(*iter, *iter2)) {
            iter = shapes.erase(iter); 
            end = shapes.end(); 
            erased = true;
            break;
        }
        ++iter2;
    }
    if (!erased)
        ++iter;
}

Alternatively, maybe something more like this would also work:

shapes.erase(
  std::remove_if(shapes.begin(), shapes.end(),
    [shapes&](Shape *i) {
        for (Shape *j : shapes) {
            if ((i != j) && check(i, j)) {
                return true;
            }
        }
        return false;
    }
  ),
  shapes.end()
);
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • Could I also do iter2 = shapes.erase(iter2) below the first erase? – user1234 Dec 12 '16 at 21:36
  • You are only operating on one `vector`, so you have to be very careful with how you manage the iterators. Doing a second `erase` will affect the new iterators obtain from the first `erase`. Do you really want to remove multiple elements per loop iteration? – Remy Lebeau Dec 12 '16 at 22:16
  • yeah once two objects return true after the check they both need to be removed – user1234 Dec 12 '16 at 22:17
  • That gets a bit trickier to manage when using iterators. You might have to switch to indexes instead. It would help to know what `check()` is actually checking, and whether `j` can ever be before `i` in the check result, or will `j` always be after `i`. – Remy Lebeau Dec 12 '16 at 22:24