Using range-based loop on QVector

SeDi

Hi,
trying to iterate over a QVector with a range-based for like this:

    for (auto p : m_dataSets.at(i)->dataPoints()) {
        drawXShape(painter, zoom*p->x(), p->y(), 5);
    }

gives out a warning:

c++11 range-loop might detach Qt container (QVector) [clazy-range-loop]

Following this thread I have tried to use qAsConst to solve this:

    for (auto p : qAsConst(m_dataSets.at(i)->dataPoints())) {
        drawXShape(painter, p->x(), p->y(), 5);
    }

Unfortunately this seems impossible:

error: call to deleted function 'qAsConst'
candidate function [with T = QVector<DataPoint *>] has been explicitly deleted
candidate function [with T = QVector<DataPoint *>] not viable: expects an l-value for 1st argument
error: cannot use type 'void' as a range

So I had to go for the (imho fairly ugly) QVectorIterator:

    QVectorIterator<DataPoint *> iter(dataSets().at(0)->dataPoints());
    while (iter.hasNext()) {
        drawXShape(painter, zoom*iter.peekNext()->x(), iter.peekNext()->y(), 5);
        iter.next();
    }

Am I missing something or is QVector just incompatible with range-based for?

Chris Kawa

is QVector just incompatible with range-based for?

It most certainly is compatible. qAsConst is deliberately deleted for r-value references, meaning your dataPoints() function returns temporary. What is its signature? If it returns a copy then you already have an inefficiency and how you construct your loop doesn't matter. It should return either a const reference, in which case you don't need any casting, or a non-const reference, in which case you'd need that qAsConst to avoid detaching.

JKSH

@SeDi said in Using range-based loop on QVector:

Following this thread I have tried to use qAsConst to solve this:

You are on the right path. You need to use a const QVector with range-for, but as @Chris-Kawa explained qAsConst() can't be used with temporary variables.

Here's another way to provide a const QVector:

const auto dataPoints = m_dataSets.at(i)->dataPoints();
for (auto p : dataPoints) {
    drawXShape(painter, zoom*p->x(), p->y(), 5);
}

Or, you can make your method return a const QVector (example: const QVector<DataPoint*> DataSet::constDataPoints() const;) and pass that directly into the range-for:

for (auto p : m_dataSets.at(i)->constDataPoints()) {
    drawXShape(painter, zoom*p->x(), p->y(), 5);
}

@Chris-Kawa said in Using range-based loop on QVector:

If it returns a copy then you already have an inefficiency and how you construct your loop doesn't matter. It should return either a const reference...

Disagree: QVector is copy-on-write so returning a copy only increments an internal refcount which is efficient enough. Returning a copy is always safe; returning a reference can be dangerous in some scenarios.

Chris Kawa

@JKSH said:

a copy only increments an internal refcount which is efficient enough

Efficient enough for some cases, not in others. Increasing a refcount atomically is not a lot of work but is still more work than doing nothing, and sometimes that difference matters.

Returning a copy is always safe. Returning a reference can be dangerous

I could easily make an argument for the reverse. I've fixed a lot of bugs where someone forgot/didn't think to return a & and scratched their heads why they modify a returned container but the changes don't propagate. It depends. Bugs are possible and common in even the simplest of code.

JKSH

@Chris-Kawa said in Using range-based loop on QVector:

@JKSH said:

a copy only increments an internal refcount which is efficient enough

Efficient enough for some cases, not in others. Increasing a refcount atomically is not a lot of work but is still more work than doing nothing, and sometimes that difference matters.

In cases where this matters, then Qt containers should probably be avoided anyway -- and the OP's original question would no longer apply.

Returning a copy is always safe. Returning a reference can be dangerous

I could easily make an argument for the reverse. I've fixed a lot of bugs where someone forgot/didn't think to return a & and scratched their heads why they modify a returned container but the changes don't propagate. It depends. Bugs are possible and common in even the simplest of code.

No arguments about the possibility of bugs, regardless of approach.

Pros and cons of each, I suppose. I learnt C++ through Qt so I've adopted the Qt API's traditional tendency to prioritize safety and ease-of-use.

SeDi

Thanks for the insight, guys - absolutely helpful!

Solved Using range-based loop on QVector