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

Use extend instead of pushing in a loop #10459

Closed
wants to merge 1 commit into from

Conversation

notriddle
Copy link
Contributor

No description provided.

@rust-highfive
Copy link

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2022
@alexcrichton
Copy link
Member

Thanks for the PR, does this show up in profiling? Or otherwise how were these found? If it's just changing code for changing sake I might say we shouldn't do this?

@notriddle
Copy link
Contributor Author

I haven't bencharked it, no.

Mostly, it just seems like code cleanup: if it can be phrased as a collect(), it should be, even if it doesn't make things faster, because it better reflects the code's intent.

@alexcrichton
Copy link
Member

Hm ok well personally I do not agree and do not want to merge this PR. If others on the Cargo team feel differently they can r+, otherwise I'll close this if no one else wants to pick this up.

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 8, 2022

I do like the collect over the push in a loop. But don't know if it is worth the code cherne.

@weihanglo
Copy link
Member

I merged one (#10453) days ago 😅
Although I like the change, I'd suggest not to submit too much this kind of PR unless the readability or performance gains a lot, since it's more like a personal preference.

@alexcrichton
Copy link
Member

Thanks for the PR, but I'm going to be stepping down from the Cargo team so I'm going to un-assign myself from this. The Cargo team will help review this when they get a chance.

@alexcrichton alexcrichton removed their assignment Mar 8, 2022
@notriddle notriddle closed this Mar 8, 2022
@notriddle
Copy link
Contributor Author

Although I like the change, I'd suggest not to submit too much this kind of PR unless the readability or performance gains a lot, since it's more like a personal preference.

That's fine. I understand the concern about wanting to keep the git history readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants