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

[opencode]

@JonB
I am sorry, forgot to paste the code. I don't understand how to fix it from the warning message.

void Scanner::run()
{
    qDebug() << "thread" << QThread::thread()->currentThreadId() << "started";

    while (m_bRunning)
    {

        QSet<QString> arrFiles = QDir(m_strPath).entryList({"*.txt","*.dat"}).toSet();

        // report all changes and store the current state
        for (const QString &strFile : (arrFiles - m_arrLastFiles))
        {
            emit Change(QString(tr("File %1 was added")).arg(strFile));
        }
        for (const QString &strFile : (m_arrLastFiles - arrFiles))
        {
            emit Change(QString(tr("File %1 was removed")).arg(strFile));
        }
        m_arrLastFiles = arrFiles;

        QThread::sleep(1);
    }
}

[JonB]

@opencode
Well, not that I have tried it, but I believe it is asking you change the right-hand side range expression in your loops:

for (const QString &strFile : qAsConst(arrFiles - m_arrLastFiles))

for (const QString &strFile : qAsConst(m_arrLastFiles - arrFiles))

Without the qAsConst() marker the danger is that those expressions will cause a "detach" --- i.e. copy --- of the elements comprising the range. This would still work, but is inefficient: the compiler knows from your const iterators that you will only be reading. not altering the range elements so suggests you insert the qAsConst() which will allow the range elements to be accessed in the original container without unnecessary copying.

[Paul Colby]

Hi @opencode,

@JonB is correct, those two lines will cause unnecessary deep copies.

Unfortunately, you can't fix that warning with qAsConst:

because it cannot be efficiently implemented for rvalues without leaving dangling references.

For this reason Qt deletes the relevant template instantiation. So, you're left with either explicitly casting the temporary to be const, or assign to a const variable.

// Warning: c++11 range-loop might detach Qt container (QSet)
for (const QString &strFile : (arrFiles - m_arrLastFiles));

// Error: Call to deleted function 'qAsConst'
for (const QString &strFile : qAsConst(arrFiles - m_arrLastFiles));

// Compiles ok.
for (const QString &strFile : static_cast<const QSet<QString>>(arrFiles - m_arrLastFiles))

// Ok.
const auto set = (arrFiles - m_arrLastFiles);
for (const QString &strFile : set);

Cheers.

[JonB]

@Paul-Colby
Thanks for clarifying, Paul. Shame the error message here suggests something which you cannot actually do :)

I am just musing on this. If I understand correctly, the two variables in the range are QSets and - (minus) returns the difference set (first set with second set's items removed). I'm not sure how this would ever reference the original container, I would have thought it has to produce a new set anyway?

[opencode]

@Paul-Colby The warnning message in Qt Creator desapeared. Thank you. :)

[Paul Colby]

Hi @JonB,

I would have thought it has to produce a new set anyway?

That's correct, the compiler creates a temporary QSet, and that's that QSet that the warning is referring to.

Consider this pseudo-code:

for (const auto &value: (QSet<T> temporarySet = (setA - setB)));

When the compiler says "range-loop might detach Qt container (QSet)", the QSet it's referring to is the temporarySet, not the variable setA or setB.

QSet::operator-() returns a non-const QSet<T>, so even if setA and setB are both const, the temporary rvalue temporarySet is non-const, so the compiler picks the non-const QSet::begin() overload and Qt detaches to ensure the returned temporarySet (non-const) iterator can't accidentally modify setA within the loop.

By simply assigning the temporary set to a const variable first, or casting to add const, the compiler is forced to use the QSet::begin() const overload instead, which returns a QSet::const_iterator, guaranteeing that the set won't be modified inside the loop (not by the iterator, anyway), so Qt doesn't need to detach (can continue implicitly sharing the underlying data).

I hope that explains it a little better :)

Cheers.

[JonB]

@Paul-Colby said in c++11 range-loop might detach Qt container (QSet) [clazy-range-loop-detach]:

When the compiler says "range-loop might detach Qt container (QSet)", the QSet it's referring to is the temporarySet, not the variable setA or setB.

Yes, this is what I thought.

It would be interesting to see (I have not tried it) whether the end result is that the compiler actually generates any different code for const auto set = (arrFiles - m_arrLastFiles); vs auto set = (arrFiles - m_arrLastFiles); followed by the for loop in this case :)

[opencode]

@JonB Hi John, how do I find generated code in Qt Creator by compiler? I have no idea. The variable const auto set = (arrFiles - m_arrLastFiles); helped to solve the issue.

[JonB]

@opencode Hey, don't worry about it, it was idle curiosity :) Stick to what you're doing.

[opencode]

@JonB :]