CppCheck 2.15 performance cleanup #5931
Conversation
Upgrade CppCheck to 2.15 and ran it over the codebase. Tackled the most obvious performance warnings plus a few others. LOTS of std::string being passed instead of const references, along with vectors, maps, and other types which involve lots of copying
…nto CppCleanup
There was a problem hiding this comment.
He's ALIIIIIIIIVE!
...cough. Excuse my exuberance.
Someone has needed to go through and fix the pass-string-by-value Problem for quite some time - thank you very much for tackling this!
For future API design, I'd prefer if we use std::string_view for non-owning string references as it allows aliasing string data from any source (std::string, cstrings, array-of-char, etc.), neatly expresses the non-owner semantics in a much more visible way, and avoids the overhead of constructing a std::string object in cases where we don't actually need an "owned copy" of the string data. (For example, when pulling string parameters from Lua to pass to another function which is going to make an internal copy...)
There's no need to try to retrofit the existing API in this PR however; this is already a very significant improvement.
The opinionated migration to emplace_back is fine - although it tends to produce more incomprehensible error messages when it fails to compile and the overhead from push_back is negligible when operating on primitive types, it otherwise has no downsides so I'm good to sign off on it.
I originally thought I was going to be able to approve this immediately, but I found a few problem spots where the const-ing and copy elision was a bit too overzealous at the cost of API readability. Other than those, I look forward to merging this PR!
|
squash / fixup before merge or rebase to master, which hopefully removes the merge commit. |
Erm, I would love to, but having just tried 2 different ways all I did was smash up my history and lose my work 👍 |
There's always git reflog, so nothing is lost, until retention limit (3 months?) has passed. I was going to suggest I could squash & merge this, but I see I'm too late. Anyway, nice to see you back @fluffyfreak |
* In summary, stop passing raw std::string! Upgrade CppCheck to 2.15 and ran it over the codebase. Tackled the most obvious performance warnings plus a few others. LOTS of std::string being passed instead of const references, along with vectors, maps, and other types which involve lots of copying
Upgraded CppCheck to 2.15 and ran it over the codebase.
Tackled the most obvious performance warnings plus a few others.
LOTS of std::string being passed by copy instead of const references, along with vectors, maps, and other types which involve lots of copying.
Switched some code to 'emplace_back' instead of 'push_back'.
None of this will make a huge difference to performance but it's no longer just throwing it away.