Skip to content

New lint: Pass references when possible for large data types? #4499

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

Closed
alxest opened this issue Sep 4, 2019 · 6 comments · Fixed by #6135
Closed

New lint: Pass references when possible for large data types? #4499

alxest opened this issue Sep 4, 2019 · 6 comments · Fixed by #6135
Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@alxest
Copy link

alxest commented Sep 4, 2019

As discussed here, it is often more efficient to pass arguments by reference instead of value.
However, current compiler does not optimize such (and there seems to be good reason for that).
Therefore, it would be appropriate to warn user on such cases.

@flip1995
Copy link
Member

flip1995 commented Sep 4, 2019

As stated here, we already have the opposite lint rust-lang/rust#52274 (comment). The code there should be reusable.

Also there was recently a blog post in TWIR or on reddit benchmarking this for small types: https://www.forrestthewoods.com/blog/should-small-rust-structs-be-passed-by-copy-or-by-borrow/ Maybe @forrestthewoods wants to do the same for huge types, so that we get a good default configuration for this lint? 😄

@flip1995 flip1995 added L-perf Lint: Belongs in the perf lint group L-suggestion Lint: Improving, adding or fixing lint suggestions good first issue These issues are a good way to get started with Clippy A-lint Area: New lints labels Sep 4, 2019
@cauebs
Copy link
Contributor

cauebs commented Oct 3, 2020

I think I'll give this one a go. I'm just now studying the codebase, and will probably need some pointers.

What should be the criteria for this? How to tell if a type is large enough? Just not being Copy is not enough, surely.

Maybe something like what's on large_enum_variant?

@ebroto
Copy link
Member

ebroto commented Oct 3, 2020

Maybe something like what's on large_enum_variant?

I think that would be a safe default. The value should be configurable, though.

Also, given the destiny of the opposite lint (#5410), I would make this lint pedantic.

@cauebs
Copy link
Contributor

cauebs commented Oct 4, 2020

If someone is willing to help a newbie, I'm having trouble getting my lint to actually run.

I've generated it with cargo dev new-lint, files were created, all fine. But I've made it just output a lint unconditionally and the stderr still matches the empty .stderr, unless I add some garbage to it.

I'm running it with env TESTNAME="pass_large_type_by_move" cargo uitest, and I can see the correct test running.

What am I missing?

@ebroto
Copy link
Member

ebroto commented Oct 4, 2020

@cauebs I think you may be missing registering your new pass here:

store.register_early_pass(|| box asm_syntax::InlineAsmX86IntelSyntax);

A previous comment suggested that code from trivially_copy_pass_by_ref should be reused here, so you should probably put your new lint in that module, and rename the module accordingly.

@cauebs
Copy link
Contributor

cauebs commented Oct 8, 2020

Would it be more appropriate to implement both in the same struct, or separately, just sharing some code from floating functions?

edit: figured it out myself. It seems fine to just merge both in the same struct. PR submitted below, any feedback is welcome ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants