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 value copying with std::move and const references #1642

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

HybridDog
Copy link
Contributor

I've applied many changes suggested by clang-tidy.

@Semphriss
Copy link
Member

What does std::move do?

I read up documentation online and apparently std::move destroys the original object (hence why you had to remove the const's), and considering some of them are passed by reference, it might have unintended side effects... (those who aren't passed by reference are copied too anyways, so little is being saved by using std::move at another point in the code)

@tobbi
Copy link
Member

tobbi commented Jan 15, 2021

https://stackoverflow.com/questions/51705967/advantages-of-pass-by-value-and-stdmove-over-pass-by-reference I have to say I'm not really knowledgeable about this but I trust clang-tidy and HybridDog to do the right thing.

@tobbi tobbi merged commit 61494c5 into SuperTux:master Jan 15, 2021
@HybridDog
Copy link
Contributor Author

HybridDog commented Jan 15, 2021

I didn't know that std::move can be used for constructor parameters before I created this Pull Request with the help of clang tidy output. I assume std::move is advantageous because an rvalue argument is moved instead of copied and the function signatures are shorter and indicate that an argument will be copied.

considering some of them are passed by reference, it might have unintended side effects...

In my changes I used either std::move or a reference function parameter but not both at once unless I made a mistake.

@HybridDog HybridDog deleted the m_clang_tidy_changes branch January 15, 2021 19:18
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.

None yet

3 participants