-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Downgrade trivially_copy_pass_by_ref to pedantic #5410
Conversation
This is one of the lints, that I hit the most when I'm using Clippy and I think, that it makes the code more readable overall. That is mainly because I can save a IMO, having a filter function is a special case, which should be just So for me this lint is more about Marking as |
Reframing this as a style lint is a slight improvement but I think it would still be better to disable by default. According to #5418, as a style lint this would be the 3rd most commonly suppressed style lint which indicates that the community has consciously decided that they don't share Clippy's opinion on the style of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the analysis in #5418. Seeing, that this lint is really often allowed, I agree to turning it into a pedantic
lint.
@bors r+ rollup |
📌 Commit 94154ca has been approved by |
🌲 The tree is currently closed for pull requests below priority 2, this pull request will be tested once the tree is reopened |
Downgrade trivially_copy_pass_by_ref to pedantic The rationale for this lint is documented as: > In many calling conventions instances of structs will be passed through registers if they fit into two or less general purpose registers. I think the purported performance benefits of clippy's recommendation are overstated. This isn't worth asking people to sprinkle code with more `*``*``&``*``&` to chase the alleged performance. This should be a pedantic lint that is disabled by default and opted in if some specific performance sensitive codebase determines that it is worthwhile. As a reminder, a typical place that a reference to a primitive would come up is if the function is used as a filter. Triggering a performance-oriented lint on this type of code is the definition of pedantic. ```rust fn filter(_n: &i32) -> bool { true } fn main() { let v = vec![1, 2, 3]; v.iter().copied().filter(filter).for_each(drop); } ``` ```console warning: this argument (4 byte) is passed by reference, but would be more efficient if passed by value (limit: 8 byte) --> src/main.rs:1:15 | 1 | fn filter(_n: &i32) -> bool { | ^^^^ help: consider passing by value instead: `i32` ``` changelog: Remove trivially_copy_pass_by_ref from default set of enabled lints
Rollup of 12 pull requests Successful merges: - #5345 (Add lint for float in array comparison) - #5406 (Fix update_lints) - #5409 (Downgrade let_unit_value to pedantic) - #5410 (Downgrade trivially_copy_pass_by_ref to pedantic) - #5412 (Downgrade inefficient_to_string to pedantic) - #5415 (Add new lint for `Result<T, E>.map_or(None, Some(T))`) - #5417 (Update doc links and mentioned names in docs) - #5419 (Downgrade unreadable_literal to pedantic) - #5420 (Downgrade new_ret_no_self to pedantic) - #5422 (CONTRIBUTING.md: fix broken triage link) - #5424 (Incorrect suspicious_op_assign_impl) - #5425 (Ehance opt_as_ref_deref lint.) Failed merges: - #5411 (Downgrade implicit_hasher to pedantic) - #5428 (Move cognitive_complexity to nursery) r? @ghost changelog: rollup
Rollup of 11 pull requests Successful merges: - #5406 (Fix update_lints) - #5409 (Downgrade let_unit_value to pedantic) - #5410 (Downgrade trivially_copy_pass_by_ref to pedantic) - #5412 (Downgrade inefficient_to_string to pedantic) - #5415 (Add new lint for `Result<T, E>.map_or(None, Some(T))`) - #5417 (Update doc links and mentioned names in docs) - #5419 (Downgrade unreadable_literal to pedantic) - #5420 (Downgrade new_ret_no_self to pedantic) - #5422 (CONTRIBUTING.md: fix broken triage link) - #5424 (Incorrect suspicious_op_assign_impl) - #5425 (Ehance opt_as_ref_deref lint.) Failed merges: - #5345 (Add lint for float in array comparison) - #5411 (Downgrade implicit_hasher to pedantic) - #5428 (Move cognitive_complexity to nursery) r? @ghost changelog: rollup
The rationale for this lint is documented as:
I think the purported performance benefits of clippy's recommendation are overstated. This isn't worth asking people to sprinkle code with more
*
*
&
*
&
to chase the alleged performance.This should be a pedantic lint that is disabled by default and opted in if some specific performance sensitive codebase determines that it is worthwhile.
As a reminder, a typical place that a reference to a primitive would come up is if the function is used as a filter. Triggering a performance-oriented lint on this type of code is the definition of pedantic.
changelog: Remove trivially_copy_pass_by_ref from default set of enabled lints