Skip to content

Conversation

@edding3000
Copy link

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?

@sonarqubecloud
Copy link


lua_newtable(L);
for (auto i : l) {
for (auto* i : l) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto* i : l) {
for (const auto* i : l) {

The original 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 with setvar()).

If we want to keep this restriction, please consider to use const modifier here.

}

for (auto z : m_ranges) {
for (auto& z : m_ranges) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto& z : m_ranges) {
for (const auto& z : m_ranges) {

Please see cppcheck's warning:

warning: src/rules_exceptions.cc,198,style,constVariableReference,Variable 'z' can be declared as reference to const

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sonarcloud also reports this.

}
}
for (auto b : from->m_ranges) {
for (auto& b : from->m_ranges) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto& b : from->m_ranges) {
for (const auto& b : from->m_ranges) {

Please see cppcheck's warning:

warning: src/rules_exceptions.cc,215,style,constVariableReference,Variable 'b' can be declared as reference to const

strlen("messages"));
yajl_gen_array_open(g);
for (auto a : m_rulesMessages) {
for (auto& a : m_rulesMessages) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto& a : m_rulesMessages) {
for (const auto& a : m_rulesMessages) {

Probably here you could use const too.

strlen("tags"));
yajl_gen_array_open(g);
for (auto b : a.m_tags) {
for (auto& b : a.m_tags) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto& b : a.m_tags) {
for (const auto& b : a.m_tags) {

Probably here you could use const too.

@edding3000
Copy link
Author

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
Copy link
Member

airween commented Nov 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.

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...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants