ProgramMemory: avoid unnecessary copy in erase_if()

[firewave]

No description provided.

[firewave]

-D__GNUC__ --check-level=exhaustive ../lib/utils.cpp

Clang 17 773,369,412 -> 772,984,251
GCC 14 794,493,015 -> 795,010,529

The example from https://trac.cppcheck.net/ticket/10765#comment:4:

Clang 17 3,988,168,871 -> 3,989,222,640
GCC 14 4,098,327,498 -> 4,100,109,857

I thought this improves things slightly. Seems like I misread the local results I had before. But maybe it will improve things if the use count thing is fixed...

[pfultz2]

This requires double iteration for ever element that will be erased.

[firewave]

Yeah - the code isn't great. I mainly opened this to raise awareness and gather feedback for a better implementation (if even possible).

[pfultz2]

The initial value should come from find_if:

for (auto it = std::find_if(mValues->begin(), mValues->end(), [](const auto& x) { return pred(x.first); }); it != mValues->end(); ++it) {
    copyOnWrite();
    if (pred(it->first))
        it = mValues->erase(it);
    else
        ++it;
}

[pfultz2]

You can move the copyOnWrite out of the loop by doing find_if outside of the for statement:

auto it = std::find_if(mValues->begin(), mValues->end(), [](const auto& x) { return pred(x.first); });
if (it == mValues->end())
    return;
copyOnWrite();
for (; it != values->end(); ++it) {
    if (pred(it->first))
        it = mValues->erase(it);
    else
        ++it;
}

[firewave]

The container behind mValues changes which renders the iterators invalid.

[pfultz2]

I dont think this extra branch is needed. Especially if you can move the copyOnWrite out of the loop.

[firewave]

The latest rework get rid of the unnecessary copy-on-writes and tries to minimizes the redundant work. Unfortunately this is not really faster at all with few conditions - needs some more looking at with more complex code though.

It exposed an issue with std::find_if though with the unexpected copies of objects which needs to be looked into.

[firewave]

It exposed an issue with std::find_if though with the unexpected copies of objects which needs to be looked into.

    auto it = std::find_if(mValues->cbegin(), mValues->cend(), [&pred](const decltype(mValues->cbegin())::value_type& entry) {
        return pred(entry.first);
    });

The actual type provided by this is std::pair<const ExprIdToken, ValueFlow::Value>. The missing const on the first type is causing the copies.

[firewave]

The actual type provided by this is std::pair<const ExprIdToken, ValueFlow::Value>. The missing const on the first type is causing the copies.

I filed an upstream ticket about detecting this: llvm/llvm-project#104572. Still need to file one about detecting it ourselves.

[firewave]

This greatly reduces the amount of copies but in the used test cases it does not translate into any visible improvements. Let's see what the selfcheck says.