Update UpdateOperations::Remove to prevent Visual Studio crash in debug mode

[jamierobertson1]

Prevents a crash in visual studio when in Debug mode when removing a sole update operation from the list. (debug assertion: "cannot increment end list iterator").

Noticed this whilst removing an update operation (UpdateOperation::remove) on visual studio whilst in debug mode. Appears VS still tries to increment the iterator after calling erase if the UpdateOperation was the last item in the list.

Re-arranging it as per this pull request overcomes the issue. Possibly a visual studio bug.

Changing:

void UpdateOperations::remove(ref_ptr<Operation> op)
{
    std::scoped_lock<std::mutex> lock(_updateOperationMutex);
    for (auto itr = _updateOperationsOneTime.begin(); itr != _updateOperationsOneTime.end(); ++itr)
    {
        if (*itr == op) itr = _updateOperationsOneTime.erase(itr);
    }
    for (auto itr = _updateOperationsAllFrames.begin(); itr != _updateOperationsAllFrames.end(); ++itr)
    {
        if (*itr == op) itr = _updateOperationsAllFrames.erase(itr);
    }
}

to:

void UpdateOperations::remove(ref_ptr<Operation> op)
{
    std::scoped_lock<std::mutex> lock(_updateOperationMutex);
    for (auto itr = _updateOperationsOneTime.begin(); itr != _updateOperationsOneTime.end();)
    {
        if (*itr == op)
            itr = _updateOperationsOneTime.erase(itr);
        else
            itr++;
    }
    for (auto itr = _updateOperationsAllFrames.begin(); itr != _updateOperationsAllFrames.end();)
    {
        if (*itr == op)
            itr = _updateOperationsAllFrames.erase(itr);
        else
            itr++;
    }
}

avoids the following Crash in VS in debug mode:

 _List_const_iterator& operator++() noexcept {
#if _ITERATOR_DEBUG_LEVEL == 2
        const auto _Mycont = static_cast<const _Mylist*>(this->_Getcont());
        _STL_ASSERT(_Mycont, "cannot increment value-initialized list iterator");
        _STL_VERIFY(this->_Ptr != _Mycont->_Myhead, "cannot increment end list iterator");
#endif // _ITERATOR_DEBUG_LEVEL == 2

        this->_Ptr = this->_Ptr->_Next;
        return *this;
    }

Type of change

• Bug fix (non-breaking change which fixes an issue)

Tested by removing vsg::UpdateOperations

[timoore]

I think the error message is correct. It would be best to use std::list::remove or std::list::remove_if here to avoid making mistakes with iterators, when they are valid, etc.

[jamierobertson1]

Yes, that is simpler. Have updated pull request to:

void UpdateOperations::remove(ref_ptr<Operation> op)
{
    std::scoped_lock<std::mutex> lock(_updateOperationMutex);
    _updateOperationsOneTime.remove(op);
    _updateOperationsAllFrames.remove(op);
}

[robertosfield]

Looks like a bug to me. Surprised no-one else has picked up on it before!

The fix looks much cleaner, which makes me wonder why this wasn't done in the first place... Will check up the history before merging.