1

I noticed that when the else if statement is being executed in this code destination, despite not being dereferenced, seems to be interpreted as the actual value it is pointing to instead of the iterator. It's like it's being automatically dereferenced. The for loop executes as you would expect, but id like done to be set to true when the begin() and end() iterators are equal. destinations is a deque of integers, global to this function.

void removeDestination(int destinationFloor)
{
    bool done = false;
    while(!done && !destinations.empty())
    {
        for (auto destination = destinations.begin(); destination != destinations.end(); ++destination)
        {
            if(*destination == destinationFloor)
            {
                destinations.erase(destination);
                break;
            }
            else if(destination == destinations.end())
            {
                done = true;
            }
        }
    }

}

Thanks for the help.

LBaelish
  • 649
  • 1
  • 8
  • 21
  • 2
    What behavior of your program caused you to think the iterator was being dereferenced there? It's a strange conclusion to reach. – user2357112 Apr 17 '16 at 02:29
  • @user2357112 when the `else if` statement is being executed, it's comparing the element pointed to by `destination` to the `end()` iterator, so this code creates an infinite loop. I agree that it's weird but I noticed in the debugger while watching the local values change in this function. – LBaelish Apr 17 '16 at 02:35

3 Answers3

5

The problem has nothing to do with destination being spuriously dereferenced. The else if branch is never taken:

else if(destination == destinations.end())

because if destination reaches the end of destinations, then the loop condition:

for (auto destination = destinations.begin(); destination != destinations.end(); ++destination)
//                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

stops the loop before the else if could be entered.

user2357112
  • 260,549
  • 28
  • 431
  • 505
3

It's generally a bad idea to modify a sequence while you're iterating it, it can cause the iterators to be invalidated leading to undefined behavior.

For a deque the erase can cause invalidation of all iterators, including the one you're using for the loop.

Edit: as pointed out in the comments, you've already developed a strategy for dealing with the invalidated iterators. It's not terribly efficient, but it should work. Your problem is in not setting the done variable properly.

The key here is to realize that you're done if you've performed the entire for loop without breaking. So you assume you're done unless you break out of the for loop.

bool done = false;
while(!done && !destinations.empty())
{
    done = true;
    for (auto destination = destinations.begin(); destination != destinations.end(); ++destination)
    {
        if(*destination == destinationFloor)
        {
            destinations.erase(destination);
            done = false;
            break;
        }
    }
}
Community
  • 1
  • 1
Mark Ransom
  • 299,747
  • 42
  • 398
  • 622
  • I understand the invalidation of iterators problem. That's the issue I was trying to get around by breaking out of iteration every time an item is deleted, and then restarting the for loop. – LBaelish Apr 17 '16 at 02:51
  • @LBaelish sorry I missed that little detail. I'll update the answer after I've thought about it a bit more. – Mark Ransom Apr 17 '16 at 03:38
2

I noticed that when the else if statement is being executed in this code destination, despite not being dereferenced, seems to be interpreted as the actual value it is pointing to instead of the iterator.

Nope. if(*destination == destinationFloor) compares the value pointed to by the iterator to the value destinationFloor. Whereas, if(destination == destinations.end()) compares two iterators.

destinations is a deque of integers, global to this function.

Global variables? Not using the standard algorithms? I would encourage you to take a look at a modern C++ book. In particular, the code you have written is a well-solved problem. See the erase-remove idiom.

Community
  • 1
  • 1
Tim
  • 1,517
  • 1
  • 9
  • 15
  • I appreciate the reference to erase-remove, this function is really a private class member function. The global variable I mentioned is just a private data member. – LBaelish Apr 17 '16 at 02:44