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

Use range loop (if possible) #7770

Open
wants to merge 1 commit into
base: master
from

Conversation

6 participants
@ShFil119
Copy link
Contributor

ShFil119 commented Feb 1, 2019

Should behave as original code, but without magic numbers and looks a bit simpler.

// not sure if formatting will satisfy you :(

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Feb 1, 2019

  • Please don't touch the code in the Externals directory.
  • There should be no space before &. (I'll run the lint bot for this PR so it can point out the incorrect formatting)
  • Don't use the m_ prefix for local variables.

@ShFil119 ShFil119 force-pushed the ShFil119:loop branch 2 times, most recently from 0000e14 to 491d36a Feb 1, 2019

@ShFil119

This comment has been minimized.

Copy link
Contributor Author

ShFil119 commented Feb 1, 2019

Should be ok now.
// one more force push, used more auto

@ShFil119 ShFil119 force-pushed the ShFil119:loop branch from 491d36a to f2a4c48 Feb 1, 2019

for (size_t i = 0; i < ArraySize(g_dsp.reg_stack); i++)
std::fill(std::begin(g_dsp.reg_stack[i]), std::end(g_dsp.reg_stack[i]), 0);
for (auto& i : g_dsp.reg_stack)
std::fill(std::begin(i), std::end(i), 0);

This comment has been minimized.

@AdmiralCurtiss

AdmiralCurtiss Feb 1, 2019

Contributor

I dunno about using i for a loop value. Granted, in this case I'm not sure what a better name would be, but that just gives the wrong impression when glancing over the code.

You do this in a few other spots too.

This comment has been minimized.

@BhaaLseN

BhaaLseN Feb 2, 2019

Member

That'd be the only shortcomings I see with this PR - most of those single-letter iteration variables could use a better name. Or least don't call them i which is the most likely one to be used in a plain for loop as counter.

for (size_t i = 0; i < ArraySize(key_map); i++)
ImGui::GetIO().KeyMap[key_map[i][0]] = (key_map[i][1] & 0x1FF);
for (auto i : key_map)
ImGui::GetIO().KeyMap[i[0]] = (i[1] & 0x1FF);

This comment has been minimized.

@AdmiralCurtiss

AdmiralCurtiss Feb 1, 2019

Contributor

Shouldn't this use auto& too?

This comment has been minimized.

@ShFil119

ShFil119 Feb 1, 2019

Author Contributor

I think it's not needed. (if you expand up, you will see that key_map is array of ints)

@ShFil119 ShFil119 force-pushed the ShFil119:loop branch from f2a4c48 to 1605fbf Feb 2, 2019

@ShFil119

This comment has been minimized.

Copy link
Contributor Author

ShFil119 commented Feb 2, 2019

Renamed a bit.

@Tilka

This comment has been minimized.

Copy link
Member

Tilka commented Feb 11, 2019

From the style guide:

Do not use the auto keyword everywhere. While it's nice that the type can be determined by the compiler, it cannot be resolved at 'readtime' by the developer as easily. Use auto only in cases where it is obvious what the type being assigned is (note: 'obvious' means not having to open other files or reading the header file). Some situations where it is appropriate to use auto is when iterating over a std::map container in a foreach loop, or to shorten the length of container iterator variable declarations.

Show resolved Hide resolved Source/Core/InputCommon/GCAdapter.cpp Outdated
Show resolved Hide resolved Source/Core/InputCommon/GCAdapter.cpp Outdated
Show resolved Hide resolved Source/Core/DolphinQt/RenderWidget.cpp Outdated

@ShFil119 ShFil119 force-pushed the ShFil119:loop branch 2 times, most recently from 7b5d23e to 55e8bb9 Feb 11, 2019

@ShFil119

This comment has been minimized.

Copy link
Contributor Author

ShFil119 commented Feb 11, 2019

Small up.

@ShFil119 ShFil119 force-pushed the ShFil119:loop branch 4 times, most recently from 3c062ee to 5035b3b Feb 11, 2019

@ShFil119 ShFil119 force-pushed the ShFil119:loop branch from 5035b3b to c254c67 Feb 11, 2019

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