Infinite for loop

[ed13]

I'm having some odd results in the following code:

while (shapes.size()>1){
        
        for (int x = 0; x < shapes.size()-1; x++){
            std::cout << x << " is x";
            std::cout << shapes.size() << " size";
            for (int y = x+1; y < shapes.size(); y++){
                std::cout << y << " y";
                if (shapes[x]->overlap(shapes[y])){
                    std::cout << "Shapes Overlap\n";
                    std::cout << "Shape " << x << " and " << y;
                    shapes.erase(shapes.begin()+x);
                    shapes.erase(shapes.begin()+(y-1));
                    y--;
                    x--;
                    
                    std::cout << "Shapes Overlap\n";
                    
                }
                
                
                
                    
            }
        }

A few things:

shapes is a Vector.
My trace shows that shapes.size() only ever decreases.

The main issue is that the outer for loop never exists and x increments towards infinity. How can this be possible when the condition x < shapes.size()-1 is broken at each iteration? For example, at one point x=5000, while shapes.size()=0. How can this occur?

Let me know if there's anything I need to provide.

Thanks.

[ed13]

This could have something to do with shapes.size()-1 underflowing. Is there a way to return a signed integer in the for loop condition?

[helios]

Why not copy the non-overlapping shapes into a new vector, rather than removing the overlapping shapes from the original vector? Modifying a container as it is being iterated is always messy.

[Ganado]

First, you should explain what you're actually trying to do in real-life terms in your code. You might just overcomplicating this, and be digging yourself a deeper hole by focusing on shapes.size()-1 not working... so, first.

I suggest reading each line of code, out loud, to a rubber duck (or other cute object): https://en.wikipedia.org/wiki/Rubber_duck_debugging

In fact, I demand you to do this. You're probably thinking it's dumb and won't help. Well, it can.

That being said, after you think about the problem and talk to a rubber duck, if you're still having trouble:
Looping on shapes.size() is a bad idea when you are mutating the shapes vector itself within the loop, unless you're being careful. If shapes.size() is 0, shapes.size()-1 becomes the largest possible size_t (an unsigned integral type).
Perhaps save the shapes.size() variable into a variable before the loop so that you know you're working with a constant size for each iteration?

Other thoughts:
Why not loop from x = 1 to shape.size() and then offset everything by -1 so that you know you're not going to hit a negative number? Don't do shapes.size()-1 unless you absolutely can't think of any other way.
When you erase things, your vector size changes. Have a fullproof way to keep track of this.

Also, these algorithms remove_if and remove, might help. http://en.cppreference.com/w/cpp/algorithm/remove
Unless you're doing this for an assignment, and aren't allowed to use certain parts of the standard library, then it really should be your first resource to avoid reinventing the wheel.

[TheIdeasMan]

Hi,

I think helios suggestion goes to the heart of the problem.

The size functions in STL containers return a std::size_t , so you should get a warning about mismatched types when comparing to an int. The fact that std::size_t is unsigned might be a clue to your problem.

I always use one of the unsigned types if it suits the data. For example, I wouldn't use int for a DaysPerWeek variable.

[ed13]

I've found that a simple (int) cast does the trick and stops size from underflowing.

As is, would you say this code is well written (ignoring the nonsensical couts)? Pseudo-code-wise, the logic seems to be sound.

The purpose is to find and remove overlapping shapes in a Vector of shapes. I've done this by comparing every shape (the for loops).