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

Remove unnecessary usages of the vec! macro #680

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Sep 21, 2023

For cases where a known-at-compile-time length array is being passed to a function that accepts an Into<Iterator>, there is no need to wrap the array in a vec![] invocation.

I spotted a couple of these in one of the newly added libcnb-test tests, and a search and replace later it turns out there were quite a few more instances of this than I was expecting!

GUS-W-14160950.

Copy link
Member

@Malax Malax left a comment

Choose a reason for hiding this comment

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

Unsure if I'd agree that vec![] is worse than Vec::new() when we need to create an empty Vec. Seems insignificant to me. At the same time if you feel strongly about this I'm happy to approve.

👍🏻 on the changes where we save allocating a Vec! :)

For cases where a known-at-compile-time length array is being
passed to a function that accepts an `Into<Iterator>`, there is
no need to wrap the array in a `vec![]` invocation.

I spotted a couple of these in one of the newly added libcnb-test
tests, and a search and replace later it turns out there were quite
a few instances of this!
@edmorley edmorley force-pushed the edmorley/rm-unnecessary-vec-macros branch from 1726b0d to 4a8bc04 Compare September 21, 2023 10:44
@edmorley edmorley merged commit 15b19e2 into main Sep 21, 2023
4 checks passed
@edmorley edmorley deleted the edmorley/rm-unnecessary-vec-macros branch September 21, 2023 10:50
@edmorley
Copy link
Member Author

edmorley commented Sep 21, 2023

More context from Slack:

[Ed] I can see an argument for using either. The reasons I lean towards Vec::new() are that:

  • Most types don't have a macro form, and instead only have the ::new() variant, eg HashMap::new(), so it seems more consistent to always use the ::new() form
  • The vec! macro to me implies "we're creating a constant length non-empty thing at compile time, otherwise we'd be dynamically pushing into a vec at runtime". eg like String::new("") would be weird to use vs String::new()
  • Fewer macros means less for the compiler to do (I don't expect this change to make any difference, but as general principal if there's two options that are virtually identical from a verbosity/UX point of view and one means using a macro and the other doesn't, then I think I would generally pick the non-macro option)

There are some additional thoughts in https://www.reddit.com/r/rust/comments/fs1g5h/vec_or_vecnew/

I didn't originally intend to make the additional vec![] -> Vec::new() changes but it got picked up by the search and replace (of the outright vec! removals), so I went with it (since I've thought about changing them before, but decided not worth the effort on its own)

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

Successfully merging this pull request may close these issues.

2 participants