Skip to content

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Sep 21, 2022

This adds a box-default lint to suggest using Box::default() instead of Box::new(Default::default()), which offers less moving parts and potentially better performance according to the perf book.


changelog: add [box_default] lint

@rust-highfive
Copy link

r? @giraffate

(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 Sep 21, 2022
@llogiq
Copy link
Contributor Author

llogiq commented Sep 25, 2022

r? @Alexendoo (as giraffate seems to be busy at the moment)

@rust-highfive rust-highfive assigned Alexendoo and unassigned giraffate Sep 25, 2022
Comment on lines +18 to +22
/// The lint may miss some cases (e.g. Box::new(String::from(""))).
/// On the other hand, it will trigger on cases where the `default`
/// code comes from a macro that does something different based on
/// e.g. target operating system.
Copy link
Member

Choose a reason for hiding this comment

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

Could add a check for expr.span.ctxt() == arg.span.ctxt() to avoid linting across macro boundaries while still catching it in local macro definitions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.

Copy link
Member

Choose a reason for hiding this comment

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

Can remove the macro note from Known problems with that unless there's a case I'm missing

Copy link
Member

Choose a reason for hiding this comment

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

Oh maybe things like Box::new(cfg_if! ... { String::new() } else { ... }) where the expressions aren't coming from the macro

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. Though if you want, I can remove the note until I find a clearer wording.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say remove it, apparently you can't use cfg_if in an expression position, nor #[cfg] as attributes on expressions are unstable

@llogiq
Copy link
Contributor Author

llogiq commented Sep 27, 2022

Got it! r?

@Alexendoo
Copy link
Member

Ahh right the parent of Box::new would probably be the impl Box {} not Box itself

@bors r+

@bors
Copy link
Contributor

bors commented Sep 27, 2022

📌 Commit 63f441e has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 27, 2022

⌛ Testing commit 63f441e with merge 9aa85dc...

@bors
Copy link
Contributor

bors commented Sep 27, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 9aa85dc to master...

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