- Notifications
You must be signed in to change notification settings - Fork1.7k
Performance tweaks#3468
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:v3/master
Are you sure you want to change the base?
Performance tweaks#3468
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/engine/lua.cc Outdated
| lua_newtable(L); | ||
| for (auto i : l) { | ||
| for (auto* i : l) { |
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.
| for (auto* i : l) { | |
| for (constauto* i : l) { |
Theoriginal code was written about 8 years ago, the author is no longer involved in development.
If I had to guess why they used value copy mechanism, I would say they wanted to avoid any chance to modify the value on Lua stack (except for intentional change withsetvar()).
If we want to keep this restriction, please consider to useconst modifier here.
src/rules_exceptions.cc Outdated
| } | ||
| for (auto z : m_ranges) { | ||
| for (auto& z : m_ranges) { |
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.
| for (auto& z : m_ranges) { | |
| for (constauto& z : m_ranges) { |
Please seecppcheck'swarning:
warning: src/rules_exceptions.cc,198,style,constVariableReference,Variable 'z' can be declared as reference to constThere 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.
Sonarcloud also reportsthis.
src/rules_exceptions.cc Outdated
| } | ||
| } | ||
| for (auto b : from->m_ranges) { | ||
| for (auto& b : from->m_ranges) { |
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.
| for (auto& b : from->m_ranges) { | |
| for (constauto& b : from->m_ranges) { |
Please see cppcheck'swarning:
warning: src/rules_exceptions.cc,215,style,constVariableReference,Variable 'b' can be declared as reference to constsrc/transaction.cc Outdated
| strlen("messages")); | ||
| yajl_gen_array_open(g); | ||
| for (auto a : m_rulesMessages) { | ||
| for (auto& a : m_rulesMessages) { |
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.
| for (auto& a : m_rulesMessages) { | |
| for (constauto& a : m_rulesMessages) { |
Probably here you could useconst too.
src/transaction.cc Outdated
| strlen("tags")); | ||
| yajl_gen_array_open(g); | ||
| for (auto b : a.m_tags) { | ||
| for (auto& b : a.m_tags) { |
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.
| for (auto& b : a.m_tags) { | |
| for (constauto& b : a.m_tags) { |
Probably here you could useconst too.
edding3000 commentedNov 20, 2025
I was not sure if there was any restriction not to use const, thats why i asked. I could extend the PR with more positions for "const auto&" if you like. |
airween commentedNov 20, 2025
Sure, go! If the tests will be passed, I'll be happy :). Btw you could review the tests too, may be you will find some lack there too (for eg. you're checking the code...) |
with changes frome9bf9ad nothing is modified anymore
what
Range based for loops with references are already used in this project, but in a few places not.
I am not sure why. I changed this code locations and executed unit tests.
Also use '*' for pointers when using auto keyword, makes it more readable.
why
Improve performance a little bit.
questions
"const auto&" could also be used in many places. Is there a reason why it is not used?