Skip to content
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

Remove loop* and loopv* macros from menus.cpp #175

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MoonPadUSer
Copy link
Contributor

Basically just doing a little bit of refactoring by getting rid of all loop macros in src/engine/menus.cpp

@robalni
Copy link
Contributor

robalni commented Jul 28, 2020

You're using a different code style, '{' on same line and space after if and for.

@MoonPadUSer
Copy link
Contributor Author

@robalni Indeed, I do. Because:

for (int i = 0; i < 2; i++) {
    printf("This is fine\n");
}

saves space and is just as easy to read as

for (int i = 0; i < 2; i++)
{
    printf("This is great too!\n");
}

I only like to do that when there's only one line inside the curly brackets (I hope I didn't violate that rule in here)

@robalni
Copy link
Contributor

robalni commented Jul 28, 2020

I prefer to have everything the same, otherwise it will look like a mess after a while.

@MoonPadUSer
Copy link
Contributor Author

Hum, ok, can't hurt. :) will change it

@MoonPadUSer
Copy link
Contributor Author

alright, now it should be good.

@blue-nebula blue-nebula locked and limited conversation to collaborators Jul 28, 2020
@blue-nebula blue-nebula unlocked this conversation Jul 28, 2020
@@ -850,7 +863,10 @@ static struct applymenu : menu
g.start(menu_start, NULL, true);
g.text("the following settings have changed:");
g.pushfont("little");
loopv(needsapply) g.text(needsapply[i].desc, 0xFFFFFF, "point");
for (int i = 0; i < needsapply.length(); i++)
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest not to just stubbornly replace all occurrences of the macro, but just go for the more modern iterator-based loops where applicable. How about

Suggested change
for (int i = 0; i < needsapply.length(); i++)
for (const auto& i : needsapply)

If you can not use this approach, please don't use the deprecated length() but the STL-compatible size().

Applies to pretty much all loops you modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll do that

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

Successfully merging this pull request may close these issues.

3 participants