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,

is this clazy warning something to worry about?
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]