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

[TRIVIAL] Fix unneeded copies in range some range for loops: #3316

Closed
wants to merge 1 commit into from

Conversation

seelabs
Copy link
Collaborator

@seelabs seelabs commented Mar 25, 2020

clang 10 warns about an unneeded copy for these range for
loops (range-loop-construct warnings)

cjcobb23
cjcobb23 previously approved these changes Mar 25, 2020
Copy link
Contributor

@cjcobb23 cjcobb23 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Left one nit.

@@ -946,7 +946,7 @@ struct PayChan_test : public beast::unit_test::suite

Env env(*this);
env.fund(XRP(10000), alice);
for (auto const a : bobs)
for (auto const& a : bobs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can you change a to b here? Same for line 993. Since the collection is called bobs, I think b is a better name. Also, on line 959, we iterate through bobs and name the element b.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a for "account", but I don't mind changing it to b. Although that may take this out of the running for shortest patch - four characters!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha! It's your call then. I don't mind either way.

@seelabs seelabs added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 27, 2020
clang 10 warns about an unneeded copy for these range for
loops (range-loop-construct warnings)
This was referenced Apr 7, 2020
@manojsdoshi manojsdoshi mentioned this pull request Apr 8, 2020
@seelabs seelabs deleted the clang-warnings branch April 24, 2020 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants