Removing items in a certain range from a list.

[gws923]

Hello all, I'm a bit stuck with this one.
I have a list that holds "Points," a class I have created. Some of these points are designated as "loop1 points" (the beginning point of a loop) and some as "loop2 points" (the end of a loop).

I have a RemovePoint() method which works when removing individually selected points. However, if the selected point is a loop1 point, I would like it to remove that point as well as all the other points between it and including the next loop2 point that shows up. What I have compiles but crashes at runtime.

void PointManager::RemovePoint(int id)
{
    for(itr=PointList.begin(); itr != PointList.end(); ++itr)
    {
        if((*itr).GetId() == id)
        {
            //----------------------------------------------
            //the offending code is below
            if((*itr).GetType() == "loop1")
            {
                std::list<Point>::iterator loopItr;
                for(loopItr = itr; (*loopItr).GetType() != "loop2"; ++loopItr)
                {
                    if(loopItr != PointList.end())
                    {
                        loopItr = PointList.erase(loopItr);
                    }
                }

                numLoopPoints -= 2;
                numLoops--;
            }
            //end offending code.  everything below works as desired.
            //----------------------------------------------
            else if((*itr).GetType() == "loop2")
            {
                numLoopPoints--;
                itr = PointList.erase(itr);
                _cManager.RemoveConnector(id);
            }
            else
            {
                itr = PointList.erase(itr);
                _cManager.RemoveConnector(id);
            }

            if(currentId != 0){currentId = id - 1;}
            else{currentId = PointList.back().GetId();}   //if the currentId point is the front of the list, cycle currentId to the back.
        }
    }
}

Does anyone have any insight? I would be happy to provide more code or explanation if necessary. Thank you in advance, I will owe you one!

[Cubbi]

On the first glance the offending loop dereferences the iterator before testing if it is end(), double-advances loopItr on erase, and trashes itr.

But why use so many loops anyway?

    // scan for points with the desired id
    for(auto i  = find_if(PointList.begin(), PointList.end(), HasId(id));
             i != PointList.end();
             i  = find_if(i, PointList.end(), HasId(id)) )
    {
        if(i->GetType() == "loop1") {
            // scan for the next loop2
            auto loopItr = find_if(i, PointList.end(), HasType("loop2"));
            if(loopItr != PointList.end())  {
                ++loopItr; // include the loop2 in the range
                i = PointList.erase(i, loopItr); // erase from now to loop2
                continue;
            }
        } // end if loop1
         ++i; 
    } // end scan for id 

where HasType and HasId are either lambdas such as

[id](const Point& p) {
    return p.GetId() == id;
}

or equivalent functors such as

struct HasId
{
    int id;
    HasId(int id) : id(id) {}
    bool operator()(const Point& p) const   {
       return p.GetId() == id;
    }
};

[gws923]

Why use so many loops, you ask?
Because I had no idea I could do it the way you just did it :)

I'm still getting my feet wet in c++ (and programming in general). I will see if I can make sense of this and finagle it to work. Thank you so much!

EDIT: My compiler seems not to like "auto." Do I need to include something to make this work? I'm getting the compile error:

"ISO C++ forbids declaration of 'i' with no type."

I know that the point of auto is that I can not declare the type... what's the deal?

[gws923]

Just thought I'd post a separate thought so as to leave the above up for anyone else who might encounter a similar problem(?)

I change the "auto" back to std::list<Point>::iterator itr and everything works great!

So thank you for the help! You've saved my neck!

[firedraco]

auto is part of C++11 so your compiler probably doesn't support it. You might be able to turn it on through a switch or something though.