C++ - std::list.erase() not removing element

Question

I am having a small issue with a bit of code for a school assignment (I know that's shunned upon here, but I locked myself into using the std::list library and am paying for it). I have a function that has a list of pointers to classes passed into it along with a particular ID belonging to one of those classes that I want to destroy and resize my list. However, with my code, the list is never resized and the values are garbage, that crash my program. So it looks like the actual class is being removed, but the element is never removed from the list...

If I had time to make my own doubly-linked list implementation, I'd iterate over the list looking for the element that I want to delete. If it is found, create a temporary node pointer and point that to the node I am about to delete. Set the previous node's "next" element to the iterator's "next" element, and then delete the iterator node.

But.. using the stl::list implementation, I'm at a loss what to do. Here is what I have so far, where a DOCO is a class, and the elements in the list are pointers to instances of classes. I've looked into remove() vs. erase(), which maybe using both may fix it, but I'm not sure how to implement remove() with an iterator like this.

bool DOCO::kill_doco(std::list < DOCO* > docolist, int docoid)
{
    for (std::list<DOCO*>::iterator it = docolist.begin(); it != docolist.end(); )
    {
        if ((*it)->id == docoid)
        {
            delete * it;
            it = docolist.erase(it);
            std::cerr << "item erased\n";
        }
        else
        {
            ++it;
        }
    }
    std::cerr << "leaving kill\n";
    
    return true;
}

Can you provide us with a [mcve] that reproduces the behavior please? At a 1st glance, your code looks just fine. — πάντα ῥεῖ, Oct 10 '20 at 21:07
Don't declare local 'delete * it' in the top conditional block as you try to erase it from the list and it is obviously not there. See the example in https://en.cppreference.com/w/cpp/container/list/erase — Ian4264, Oct 10 '20 at 21:10
The list from where you are erasing is a copy of the original as the function is called with pass-by-value. — Wander3r, Oct 10 '20 at 21:10
Ah wait, I was to quick. You're working on a copy of your list. You'll have to passi it by reference: `bool DOCO::kill_doco(std::list < DOCO* >& docolist, int docoid)` — πάντα ῥεῖ, Oct 10 '20 at 21:11
*"a school assignment (I know that's shunned upon here"* -- asking for someone to do your homework is shunned, but asking a specific question that arose out of your homework is fine. — JaMiT, Oct 10 '20 at 21:12
If the list owns its objects consider making it a list of the objects themselves instead of pointers. Or using std::unique_ptr or std::shared_ptr instead of raw pointers. — Zan Lynx, Oct 10 '20 at 21:25
YUP! @krisz that did help. Thank you, folks! EDIT: I also removed the delete function, that was another issue. Also, noted @JaMiT! — TheN00bBuilder, Oct 10 '20 at 21:28

score 3 · Accepted Answer · answered Oct 10 '20 at 21:12

kill_doco(std::list < DOCO* > docolist

this creates a copy of the list. This copy is a list of pointers.

You proceed to modify the copy of the list, and delete an element in it.

The original list (which you copied) still has the original pointer, which is now pointing to a deleted object.

The easy fix is:

kill_doco(std::list < DOCO* >& docolist

C++ is a value-oriented language, unlike languages like Java or C#. The name of something refers to an actual value of that thing, not a reference to it.

Pointers are similarly the value of the address of the object.

Reference like semantics, or pointer-like semantics, can be done in C++. But, unlike Java/C#, by default every object in C++ is an actual value.

People who move from one language to the other (either way) can get confused by this.

The "default" object type in a C++ program is a regular type -- a type that acts like an integer when you copy it around and the like. It is relatively easy to move away from this, but that is the default.

So what you did was akin to:

void clear_bit( int x, int bit ) {
  x = x & ~(1 << bit);
}

and being surprised that the value x you passed in wasn't modified by the function. The "dangling" pointer left in the original list is the 2nd thing that bit you.

OH! That was definitely it. I haven't done much C++ until recently, which would also explain why I missed that (mostly been doing assembly work). You and @krisz are life-savers. Now my list is crashing but I have a feeling I know why given your explanation. Thank you!! — TheN00bBuilder, Oct 10 '20 at 21:27
@TheN00bBuilder, if you had used `std::unique_pointer`, rather than raw pointers with `new`/`delete`, the compiler would have helped you here. Try to use `std::shared_ptr`, `std::unique_ptr` or other container classes rather than `new`/`delete`. Actually in modern c++ you don't need to use `new`/`delete` *at all*, except to implement smart pointers. You would be surprised on how much simpler and less buggy your code will turn out. — HAL9000, Oct 10 '20 at 21:54

C++ - std::list.erase() not removing element

1 Answers1