How to avoid mixing const_iterator and iterator when using QList::erase?

[Jack Hill]

Hi,
suppose we have a list model that has a backend m_numbers = QList<int> and we implement a function to remove rows that satisfy the predicate lowerBound <= value <= upperBound

void MyListModel::removeValues(int lowerBound, int upperBound)
{
    int row = 0;
    auto itr = m_numbers.begin();
    while (itr != m_numbers.end()) {
        if (lowerBound <= *itr && *itr <= upperBound) {
            beginRemoveRows(parent, row, row);
            itr = m_numbers.erase(itr);
            endRemoveRows();
        } else {
            itr++;
            row++;
        }
    }
}

This works fine but I have clazy complaining about mixing iterators with const_iterators due to QList::iterator QList::erase(QList::const_iterator pos). So,

1. is this clazy warning something to worry about?
2. is there a way of rewriting this function to quell the clazy warning? It seems odd to me that there is no QList::const_iterator QList::erase(QList::const_iterator pos) or QList::iterator QList::erase(QList::iterator pos)

[kshegunov]

@Jack-Hill said in How to avoid mixing const_iterator and iterator when using QList::erase?:

This works fine but I have clazy complaining about mixing iterators with const_iterators due to QList::iterator QList::erase(QList::const_iterator pos)

Then don't. This can be as simple as:

for (int i = 0, size = m_numbers.size(); i < size; i++) {
    auto value = m_numbers[i];
    if (value < lowerBound || value > upperBound)
        continue;
    beginRemoveRows(parent, i, i); // for whatever `parent` is here, it isn't clear where it comes from
    m_numbers.removeAt(i--);
    size--;
    endRemoveRows();
}

If you want to minimize the number of draws the view does on the other hand you'd do two passes - take the bounds of elements to be removed and only then remove them in batches.

[Edit: Fixed wrong boolean operation ~kshegunov]

[mpergand]

@Jack-Hill said in How to avoid mixing const_iterator and iterator when using QList::erase?:

I have clazy complaining about mixing iterators

Try using cbegin() and cend()

I faced similar issues some time ago:

 /*
          1) Use cbegin and cend to be explicit about using const iterator.
          2) begin() and end() return const_iterator when method is declared as const
      */

                typedef typename std::vector<E*>::const_iterator const_node_it;
                 const_node_it  begin() const
                            {
                            return _list.cbegin();
                            }

                const_node_it  end() const
                            {
                            return _list.cend();

                                }

[JoeCFD]

@Jack-Hill your code is not efficient and may be dangerous. Better to loop in reverse order when items are deleted.

[JonB]

@Jack-Hill
Just an observation about algorithm, not your issue. You call begin/endRemoveRows() once to remove one row at a time. I would prefer to call to once only with the range to be removed. That may help efficiency if things act on it.

[Jack Hill]

@mpergand I don't think that would work because erase returns a non-const iterator

@JoeCFD You mean something like this?

int row = m_numbers.size() - 1;
auto itr = m_numbers.end();
while (itr != m_numbers.begin()) {
    if (lowerBound <= *itr && *itr <= upperBound) {
        beginRemoveRows(parent, row, row);
        itr = m_numbers.erase(itr);
        endRemoveRows();
    } else {
        itr--;
        row--;
    }
}

[Jack Hill]

@JonB True. Would probably be better to remove consecutive items in one go instead of one-by-one.

Also I just realised I think it would be better not bother with iterators and just use indices directly like so (not tested)

for (int row = 0; row < m_numbers.size(); ) {
    const int num = m_numbers[row];
    if (lowerBound <= num && num <= upperBound) {
        beginRemoveRows(parent, row, row);
        m_numbers.remove(row);
        endRemoveRows();
    } else {
        row++;
    }
}

[JonB]

@Jack-Hill said in How to avoid mixing const_iterator and iterator when using QList::erase?:

Also I just realised I think it would be better not bother with iterators and just use indices directly like so (not tested)

:) Don't know what to say. Then this topic wouldn't exist. All these iterators and consts. I was a C guy, it was just so much easier! I have to look up iterators each time, and don't use them if I feel I don't have to/can't be bothered. But I'm sure the C++ experts around these days will recommend you do.

[JoeCFD]

@Jack-Hill index is ok. But your iterator does not match it.
https://doc.qt.io/qt-5/qlist.html#crbegin

[Jack Hill]

@JoeCFD I don't think it's possible to use reverse iterators because there's no matching erase function.

@JonB Indeed. This is from an existing code-base, so I didn't immediately think of switching to indices.

[JoeCFD]

@Jack-Hill

#include <QList>

#include <iostream>

int main()
{
    QList< int > list{ 1,2,3,4,5,6,7,8,9,10 };

    auto iter = list.rbegin();
    while( iter != list.rend() ) {
        if ( 0 == *iter % 3 ) {
            list.erase( std::next( iter ).base() );
        }
        
        iter++;
    }
    
    for ( auto const value : list ) {
        std::cout << value << std::endl;
    }
    
    return 0;
}

[Jack Hill]

@JoeCFD That does work but it still triggers the clazy warning. It seems to me like the safest way of imperatively erasing items from QLists is to iterate with indices.

[JoeCFD]

@Jack-Hill I did not see any warning with g++ 11.3.0 on Ubuntu 22.04.

[kshegunov]

@Jack-Hill said in How to avoid mixing const_iterator and iterator when using QList::erase?:

This works fine but I have clazy complaining about mixing iterators with const_iterators due to QList::iterator QList::erase(QList::const_iterator pos)

Then don't. This can be as simple as:

for (int i = 0, size = m_numbers.size(); i < size; i++) {
    auto value = m_numbers[i];
    if (value < lowerBound || value > upperBound)
        continue;
    beginRemoveRows(parent, i, i); // for whatever `parent` is here, it isn't clear where it comes from
    m_numbers.removeAt(i--);
    size--;
    endRemoveRows();
}

If you want to minimize the number of draws the view does on the other hand you'd do two passes - take the bounds of elements to be removed and only then remove them in batches.

[Edit: Fixed wrong boolean operation ~kshegunov]