-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ProgramMemory: avoid unnecessary copy in erase_if()
#6652
base: main
Are you sure you want to change the base?
Conversation
Clang 17 The example from https://trac.cppcheck.net/ticket/10765#comment:4: Clang 17 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... |
lib/programmemory.cpp
Outdated
continue; | ||
|
||
copyOnWrite(); | ||
mValues->erase(it->first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires double iteration for ever element that will be erased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - the code isn't great. I mainly opened this to raise awareness and gather feedback for a better implementation (if even possible).
lib/programmemory.cpp
Outdated
it = mValues->erase(it); | ||
else | ||
++it; | ||
for (auto it = values->begin(); it != values->end(); ++it) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The container behind mValues
changes which renders the iterators invalid.
else | ||
++it; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think this extra branch is needed. Especially if you can move the copyOnWrite
out of the loop.
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 |
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 |
I filed an upstream ticket about detecting this: llvm/llvm-project#104572. Still need to file one about detecting it ourselves. |
No description provided.