Deleting data while looping through a list

Question

I'm having trouble figuring out how to delete an item from a list.

Please note that I would like to perform the deletion from the advance() function. This code is just boiled down from my actual project, to try to isolate the error.

#include <iostream>
#include <list>
#include <iterator>
#include <algorithm>

using namespace std;

const int SCT_OSC_FILLED = 11;

class OrderInfo {
private: 
    std::string id;
public:
    OrderInfo(std::string a, int aStatusCode);
    std::string key();
    int statusCode;
};

OrderInfo::OrderInfo(std::string a, int aStatusCode) {
    id = a;
    statusCode = aStatusCode;
}

std::string OrderInfo::key() {
    return id;
}

std::list <OrderInfo> MasterOrders;

void testList();
void add(OrderInfo ordInfo);
void advance(OrderInfo ordInfo, std::list <OrderInfo> ::iterator& orderIter);

void testList() {
    OrderInfo o1("1", 15);
    OrderInfo o2("2", 16);
    OrderInfo o3("3", SCT_OSC_FILLED);
    OrderInfo o4("4", 17);
    OrderInfo o5("5", SCT_OSC_FILLED);
    OrderInfo o6("6", 18);

    add(o1);
    add(o1);
    add(o2);
    add(o3);
    add(o4);
    add(o5);
    add(o6);

    for (auto v : MasterOrders)
        std::cout << v.key() << "\n";
}

void add(OrderInfo ordInfo) {
    // Add to MasterOrders (if not already in list)
    bool alreadyInList = false;
    std::list <OrderInfo> ::iterator orderIter = MasterOrders.begin();
    while (orderIter != MasterOrders.end())
    {
        OrderInfo oi = *orderIter;
        alreadyInList = ordInfo.key() == oi.key(); 
        if (alreadyInList) break;
        advance(ordInfo, orderIter);
    }
    if (!alreadyInList) MasterOrders.push_front(ordInfo);
}

void advance(OrderInfo ordInfo, std::list <OrderInfo> ::iterator& orderIter) {
    bool iterate = true;
    if (ordInfo.statusCode == SCT_OSC_FILLED) {
        orderIter = MasterOrders.erase(orderIter++); // https://stackoverflow.com/a/5265561/270143
        iterate = false;
    }   
    
    if (iterate) orderIter++;
}

int main()
{
    testList();
    return 0;
}

update: I forgot to state the actual goal

my goal is to just delete SCT_OSC_FILLED ordInfos from within the advance() method (that part is important) and leave the rest. My actual project code does more than what is shown, these names of functions are just made up for this example.. there is more code in them not directly related to manipulating the list (but related to processing OrderInfo) in my actual project. My goal is to leave one copy of o1 in the list as well as o2, o4 and o6 - removing o3 and o5 because they have an SCT_OSC_FILLED OrderInfo.statusCode

You should change `orderIter = MasterOrders.erase(orderIter++);` to `orderIter = MasterOrders.erase(orderIter);` because the `++` is unnecessary. Or you can do `MasterOrders.erase(orderIter++);` without the assignment. But doing both is unnecessary. — john, Jun 09 '22 at 07:07
Isolating your problem is a good approach, which is why a [mcve] is even required. That said, what _is_ the problem you have? You don't say anything about that! The typical problem is "iterator invalidation" (search for that on cppreference.com). — Ulrich Eckhardt, Jun 09 '22 at 07:10
Somehow I doubt that the first parameter of the `advance` function should be there, let alone have deletions done based on it. In your `add` function you always pass a copy of the same element resulting in all of the elements being erased, should the condition be fulfilled for it. You may want to consider using something like `bool alreadyInList = std:any_of(MasterOrders.begin(), MasterOrders.end(), [&ordInfo](OrderInfo const& e) { return ordInfo.key() == e.key(); });` (which would require you to mark `OrderInfo::key` as `const` though. — fabian, Jun 09 '22 at 07:12
I don't see the bug, and the end of the program the list contains items 5 and 6, which seems correct given the code as it is written. If you were expecting something else then please say what that is. — john, Jun 09 '22 at 07:17
Adding an item with status code equal to SCT_OSC_FILLED causes the list to be emptied. That explains the output but is that what you intended? — john, Jun 09 '22 at 07:18
I wonder if `if (ordInfo.statusCode == SCT_OSC_FILLED) {` should actually be `if (ordIter->statusCode == SCT_OSC_FILLED) {` — john, Jun 09 '22 at 07:21
The `add` function is doing much more work than "adding". Maybe you should rename the function to something that reflects what is actually being done, or rewrite the function to do one thing -- add an order item. And to be honest, these set of function calls you're doing seem confusing and borders on spaghetti-type logic. What is the actual goal of `add` and `advance`? Can't these be broken up into what they are to do, instead of intertwining one inside of the other? — PaulMcKenzie, Jun 09 '22 at 08:11
Example: `alreadyInList = ordInfo.key() == oi.key();` -- This is simply `auto iter = std::find_if(MasterOrders.begin(), MasterOrders.end(), [&](OrderInfo& oi) { return oi.key() == ordInfo.key();});` and checking the returned iterator -- there is no need to write a loop for this. Also, you should really pass objects like `OrderInfo` by reference, not by value. — PaulMcKenzie, Jun 09 '22 at 08:20
@ycomp -- `MasterOrders.erase(std::remove_if(MasterOrders.begin(), MasterOrders.end(), [&](OrderInfo& oi) { return oi.statusCode==SCT_OSC_FILLED; }), MasterOrders.end());`. It seems you are not familiar with the STL algorithm functions, or at least reluctant to use them. They can take those entire `for` loops that erase items and condense it all down to a single function call. — PaulMcKenzie, Jun 09 '22 at 08:24
sorry I forgot to mention what the list should look like after processing, I have updated the question with it — ycomp, Jun 09 '22 at 08:42
@john `MasterOrders.erase(orderIter++);` is a bad idea. It only works for a few containers while others sometimes invalidate the iterator and then you get random crashes. — Goswin von Brederlow, Jun 09 '22 at 09:06
I'm open to any way to erase that specific `ordInfo` (i.e. matching `statusCode==SCT_OSC_FILLED`) from within the `advance()` function. This is the thing I'm trying to achieve and where I am stuck at, and the reason for the Question. I'm pretty new to `C++` so there could be obvious things I'm missing here. — ycomp, Jun 09 '22 at 09:16

KMot · Answer 1 · 2022-06-09T07:13:59.333

According to cppreference.com, "References and iterators to the erased elements are invalidated." You should get an iterator to the next element, before deleting the current element.

In the same page, cppreference gives an example:

// Erase all even numbers (C++11 and later)
    for (std::list<int>::iterator it = c.begin(); it != c.end(); ) {
        if (*it % 2 == 0) {
            it = c.erase(it);
        } else {
            ++it;
        }
    }

erase returns an iterator to the next element (end() if the removed element was the last), so "it = c.erase(it);" makes "it" to point to the next element, and there is no need for incrementing the iterator (with ++).

So you could have something like:

void advance(OrderInfo ordInfo, std::list <OrderInfo> ::iterator& orderIter) {
    if (ordInfo.statusCode == SCT_OSC_FILLED) {
        orderIter = MasterOrders.erase(orderIter);
    } else {
        orderIter++;
    }
}

You are right. Thanks! I will edit the answer. – KMot Jun 09 '22 at 07:09 — KMot, Jun 09 '22 at 07:09

score 1 · Accepted Answer · answered Jun 09 '22 at 09:21

So the problem is nothing to do with deletion from a list. Your logic is simply wrong given the stated goal.

You want to delete all SCT_OSC_FILLED items from the list when adding an item but the code you write deletes all items from the list when you add an item with SCT_OSC_FILLED. You are simply testing the wrong thing.

Change this

void advance(OrderInfo ordInfo, std::list <OrderInfo> ::iterator& orderIter) {
    bool iterate = true;
    if (ordInfo.statusCode == SCT_OSC_FILLED) {
        orderIter = MasterOrders.erase(orderIter++);
        iterate = false;
    }   
    
    if (iterate) orderIter++;
}

to this

void advance(OrderInfo ordInfo, std::list <OrderInfo> ::iterator& orderIter) {
    bool iterate = true;
    if (orderIter->statusCode == SCT_OSC_FILLED) {
        orderIter = MasterOrders.erase(orderIter);
        iterate = false;
    }   
    
    if (iterate) orderIter++;
}

Once you make that change you can see that the ordInfo parameter is unused. Add bit more cleanup and you end up with this much simpler function

void advance(std::list <OrderInfo> ::iterator& orderIter) {
    if (orderIter->statusCode == SCT_OSC_FILLED) {
        orderIter = MasterOrders.erase(orderIter);
    }
    else {
        orderIter++;
    }
}

thank you, I'm new to iterators and C++ and stl in general so this really helps — ycomp, Jun 09 '22 at 11:05
@ycomp OK so `if (orderIter->statusCode == SCT_OSC_FILLED) {` and `OrderInfo ordInfo = *orderIter; if (ordInfo.statusCode == SCT_OSC_FILLED) {` do the same thing but the first is more efficient since it avoids needlessly copying an `OrderInfo` object. I guess the bigger question now though is, is your code working or not. Maybe the real problem is something else again? — john, Jun 09 '22 at 11:44
the problem was with me rushing the minimal example code. I added the `ordInfo` argument in only in the example. The code in my project is working now. — ycomp, Jun 09 '22 at 15:42

Deleting data while looping through a list

update: I forgot to state the actual goal

2 Answers2