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

replace for(auto <var> to for(auto& <var> #760

Merged
merged 4 commits into from
Jul 21, 2022
Merged

replace for(auto <var> to for(auto& <var> #760

merged 4 commits into from
Jul 21, 2022

Conversation

aismann
Copy link
Contributor

@aismann aismann commented Jul 20, 2022

working with reference and not with copies
=> Performance issue

solve feature: #759

working with reference and not with copies
=> Performance issue
@halx99
Copy link
Collaborator

halx99 commented Jul 21, 2022

I think the raw iterator don't needs change to auto&, just c++11 for-loop needs for non-pointer element like:

for (auto& item : items)
    item.callSomeMethod();

@aismann
Copy link
Contributor Author

aismann commented Jul 21, 2022

I think the raw iterator don't needs change to auto&, just c++11 for-loop needs for non-pointer element like:

for (auto& item : items)
    item.callSomeMethod();

Correct! This one & was to much. Sorry. (removed it)

@aismann
Copy link
Contributor Author

aismann commented Jul 21, 2022

I think the raw iterator don't needs change to auto&, just c++11 for-loop needs for non-pointer element like:

for (auto& item : items)
    item.callSomeMethod();

@halx99
BTW: also lyasio.cpp has a: for (auto item : channel_eps) > can be changed to: for (auto& item : channel_eps)

At the moment a don't changing anything on thirtparty folders

@halx99
Copy link
Collaborator

halx99 commented Jul 21, 2022

The channel_ops element is store as pointer, so don't need change

@aismann
Copy link
Contributor Author

aismann commented Jul 21, 2022

The channel_ops element is store as pointer, so don't need change

I know. for (auto item : channel_eps) is no different to for (auto& item : channel_eps)
BUT: as an developer reading the sources its easier for understanding (its a ref not a copy).

@halx99
Copy link
Collaborator

halx99 commented Jul 21, 2022

Using c++11 Universal References maybe better like:

std::vector<bool> items1 = {false};
for(auto&& item : items1) 
    item = true;

const std::vector<bool> items2 = {true};
for (auto&& item : items2)
    printf("item=%d\n", (int)item);

@halx99 halx99 added this to the 1.0 milestone Jul 21, 2022
@halx99 halx99 added the enhancement New feature or request label Jul 21, 2022
@aismann
Copy link
Contributor Author

aismann commented Jul 21, 2022

Using c++11 Universal Reference maybe better like:

std::vector<bool> items1 = {false};
for(auto&& item : items1) 
    item = true;

const std::vector<bool> items2 = {true};
for (auto&& item : items2)
    printf("item=%d\n", (int)item);

Should I change ALL for (auto& <var1>: <var2>) to for (auto&& <var1>: <var2>) ?

@halx99 halx99 linked an issue Jul 21, 2022 that may be closed by this pull request
@halx99
Copy link
Collaborator

halx99 commented Jul 21, 2022

Using c++11 Universal Reference maybe better like:

std::vector<bool> items1 = {false};
for(auto&& item : items1) 
    item = true;

const std::vector<bool> items2 = {true};
for (auto&& item : items2)
    printf("item=%d\n", (int)item);

Should I change ALL for (auto& <var1>: <var2>) to for (auto&& <var1>: <var2>) ?

Yes, you can, the Universal References can handle both const or non-const containers.

@aismann
Copy link
Contributor Author

aismann commented Jul 21, 2022

Also for the 'extension' folder?

The ThirtParty folder will be untouched

@halx99
Copy link
Collaborator

halx99 commented Jul 21, 2022

Also for the 'extension' folder?

The ThirtParty folder will be untouched

Yes, but should ignore spine, fairygui, Live2D

@aismann
Copy link
Contributor Author

aismann commented Jul 21, 2022

this extensions be untouched (spine, Live2D has no such C++11 for loop):
Also some tester ( for (auto& touch : touches) ) is good enough
image

@aismann
Copy link
Contributor Author

aismann commented Jul 21, 2022

finished.

@halx99 halx99 merged commit 73153ad into axmolengine:dev Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace for(auto <var> or for(auto& <var> to for(auto&& <var> …
2 participants