Skip to content

Commit

Permalink
Rollup merge of #5410 - dtolnay:trivially, r=flip1995
Browse files Browse the repository at this point in the history
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
  • Loading branch information
flip1995 authored Apr 8, 2020
2 parents 935b45d + 94154ca commit 1e1bd51
Show file tree
Hide file tree
Showing 33 changed files with 135 additions and 139 deletions.
3 changes: 1 addition & 2 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&shadow::SHADOW_UNRELATED),
LintId::of(&strings::STRING_ADD_ASSIGN),
LintId::of(&trait_bounds::TYPE_REPETITION_IN_BOUNDS),
LintId::of(&trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF),
LintId::of(&types::CAST_LOSSLESS),
LintId::of(&types::CAST_POSSIBLE_TRUNCATION),
LintId::of(&types::CAST_POSSIBLE_WRAP),
Expand Down Expand Up @@ -1372,7 +1373,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&transmute::UNSOUND_COLLECTION_TRANSMUTE),
LintId::of(&transmute::WRONG_TRANSMUTE),
LintId::of(&transmuting_null::TRANSMUTING_NULL),
LintId::of(&trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF),
LintId::of(&try_err::TRY_ERR),
LintId::of(&types::ABSURD_EXTREME_COMPARISONS),
LintId::of(&types::BORROWED_BOX),
Expand Down Expand Up @@ -1665,7 +1665,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&mutex_atomic::MUTEX_ATOMIC),
LintId::of(&redundant_clone::REDUNDANT_CLONE),
LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION),
LintId::of(&trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF),
LintId::of(&types::BOX_VEC),
LintId::of(&types::REDUNDANT_ALLOCATION),
LintId::of(&vec::USELESS_VEC),
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/trivially_copy_pass_by_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ declare_clippy_lint! {
/// fn foo(v: u32) {}
/// ```
pub TRIVIALLY_COPY_PASS_BY_REF,
perf,
pedantic,
"functions taking small copyable arguments by reference"
}

Expand Down
2 changes: 1 addition & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2161,7 +2161,7 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
},
Lint {
name: "trivially_copy_pass_by_ref",
group: "perf",
group: "pedantic",
desc: "functions taking small copyable arguments by reference",
deprecation: None,
module: "trivially_copy_pass_by_ref",
Expand Down
1 change: 1 addition & 0 deletions tests/ui-toml/toml_trivially_copy/test.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// normalize-stderr-test "\(\d+ byte\)" -> "(N byte)"
// normalize-stderr-test "\(limit: \d+ byte\)" -> "(limit: N byte)"

#![deny(clippy::trivially_copy_pass_by_ref)]
#![allow(clippy::many_single_char_names)]

#[derive(Copy, Clone)]
Expand Down
10 changes: 7 additions & 3 deletions tests/ui-toml/toml_trivially_copy/test.stderr
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
error: this argument (N byte) is passed by reference, but would be more efficient if passed by value (limit: N byte)
--> $DIR/test.rs:14:11
--> $DIR/test.rs:15:11
|
LL | fn bad(x: &u16, y: &Foo) {}
| ^^^^ help: consider passing by value instead: `u16`
|
= note: `-D clippy::trivially-copy-pass-by-ref` implied by `-D warnings`
note: the lint level is defined here
--> $DIR/test.rs:4:9
|
LL | #![deny(clippy::trivially_copy_pass_by_ref)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: this argument (N byte) is passed by reference, but would be more efficient if passed by value (limit: N byte)
--> $DIR/test.rs:14:20
--> $DIR/test.rs:15:20
|
LL | fn bad(x: &u16, y: &Foo) {}
| ^^^^ help: consider passing by value instead: `Foo`
Expand Down
1 change: 0 additions & 1 deletion tests/ui/clone_on_copy_mut.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ pub fn dec_read_dec(i: &mut i32) -> i32 {
ret
}

#[allow(clippy::trivially_copy_pass_by_ref)]
pub fn minus_1(i: &i32) -> i32 {
dec_read_dec(&mut i.clone())
}
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/debug_assert_with_mut_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#![feature(custom_inner_attributes)]
#![rustfmt::skip]
#![warn(clippy::debug_assert_with_mut_call)]
#![allow(clippy::trivially_copy_pass_by_ref, clippy::cognitive_complexity, clippy::redundant_closure_call)]
#![allow(clippy::cognitive_complexity, clippy::redundant_closure_call)]

struct S;

Expand Down
3 changes: 1 addition & 2 deletions tests/ui/eta.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@
clippy::redundant_closure_call,
clippy::many_single_char_names,
clippy::needless_pass_by_value,
clippy::option_map_unit_fn,
clippy::trivially_copy_pass_by_ref
clippy::option_map_unit_fn
)]
#![warn(
clippy::redundant_closure,
Expand Down
3 changes: 1 addition & 2 deletions tests/ui/eta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@
clippy::redundant_closure_call,
clippy::many_single_char_names,
clippy::needless_pass_by_value,
clippy::option_map_unit_fn,
clippy::trivially_copy_pass_by_ref
clippy::option_map_unit_fn
)]
#![warn(
clippy::redundant_closure,
Expand Down
24 changes: 12 additions & 12 deletions tests/ui/eta.stderr
Original file line number Diff line number Diff line change
@@ -1,77 +1,77 @@
error: redundant closure found
--> $DIR/eta.rs:21:27
--> $DIR/eta.rs:20:27
|
LL | let a = Some(1u8).map(|a| foo(a));
| ^^^^^^^^^^ help: remove closure as shown: `foo`
|
= note: `-D clippy::redundant-closure` implied by `-D warnings`

error: redundant closure found
--> $DIR/eta.rs:22:10
--> $DIR/eta.rs:21:10
|
LL | meta(|a| foo(a));
| ^^^^^^^^^^ help: remove closure as shown: `foo`

error: this expression borrows a reference that is immediately dereferenced by the compiler
--> $DIR/eta.rs:25:21
--> $DIR/eta.rs:24:21
|
LL | all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted
| ^^^ help: change this to: `&2`
|
= note: `-D clippy::needless-borrow` implied by `-D warnings`

error: redundant closure found
--> $DIR/eta.rs:32:27
--> $DIR/eta.rs:31:27
|
LL | let e = Some(1u8).map(|a| generic(a));
| ^^^^^^^^^^^^^^ help: remove closure as shown: `generic`

error: redundant closure found
--> $DIR/eta.rs:75:51
--> $DIR/eta.rs:74:51
|
LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo());
| ^^^^^^^^^^^ help: remove closure as shown: `TestStruct::foo`
|
= note: `-D clippy::redundant-closure-for-method-calls` implied by `-D warnings`

error: redundant closure found
--> $DIR/eta.rs:77:51
--> $DIR/eta.rs:76:51
|
LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo());
| ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `TestTrait::trait_foo`

error: redundant closure found
--> $DIR/eta.rs:80:42
--> $DIR/eta.rs:79:42
|
LL | let e = Some(&mut vec![1, 2, 3]).map(|v| v.clear());
| ^^^^^^^^^^^^^ help: remove closure as shown: `std::vec::Vec::clear`

error: redundant closure found
--> $DIR/eta.rs:85:29
--> $DIR/eta.rs:84:29
|
LL | let e = Some("str").map(|s| s.to_string());
| ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `std::string::ToString::to_string`

error: redundant closure found
--> $DIR/eta.rs:87:27
--> $DIR/eta.rs:86:27
|
LL | let e = Some('a').map(|s| s.to_uppercase());
| ^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `char::to_uppercase`

error: redundant closure found
--> $DIR/eta.rs:90:65
--> $DIR/eta.rs:89:65
|
LL | let e: std::vec::Vec<char> = vec!['a', 'b', 'c'].iter().map(|c| c.to_ascii_uppercase()).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `char::to_ascii_uppercase`

error: redundant closure found
--> $DIR/eta.rs:173:27
--> $DIR/eta.rs:172:27
|
LL | let a = Some(1u8).map(|a| foo_ptr(a));
| ^^^^^^^^^^^^^^ help: remove closure as shown: `foo_ptr`

error: redundant closure found
--> $DIR/eta.rs:178:27
--> $DIR/eta.rs:177:27
|
LL | let a = Some(1u8).map(|a| closure(a));
| ^^^^^^^^^^^^^^ help: remove closure as shown: `closure`
Expand Down
3 changes: 1 addition & 2 deletions tests/ui/extra_unused_lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
unused,
dead_code,
clippy::needless_lifetimes,
clippy::needless_pass_by_value,
clippy::trivially_copy_pass_by_ref
clippy::needless_pass_by_value
)]
#![warn(clippy::extra_unused_lifetimes)]

Expand Down
8 changes: 4 additions & 4 deletions tests/ui/extra_unused_lifetimes.stderr
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
error: this lifetime isn't used in the function definition
--> $DIR/extra_unused_lifetimes.rs:14:14
--> $DIR/extra_unused_lifetimes.rs:13:14
|
LL | fn unused_lt<'a>(x: u8) {}
| ^^
|
= note: `-D clippy::extra-unused-lifetimes` implied by `-D warnings`

error: this lifetime isn't used in the function definition
--> $DIR/extra_unused_lifetimes.rs:16:25
--> $DIR/extra_unused_lifetimes.rs:15:25
|
LL | fn unused_lt_transitive<'a, 'b: 'a>(x: &'b u8) {
| ^^

error: this lifetime isn't used in the function definition
--> $DIR/extra_unused_lifetimes.rs:41:10
--> $DIR/extra_unused_lifetimes.rs:40:10
|
LL | fn x<'a>(&self) {}
| ^^

error: this lifetime isn't used in the function definition
--> $DIR/extra_unused_lifetimes.rs:67:22
--> $DIR/extra_unused_lifetimes.rs:66:22
|
LL | fn unused_lt<'a>(x: u8) {}
| ^^
Expand Down
3 changes: 1 addition & 2 deletions tests/ui/float_arithmetic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
clippy::shadow_unrelated,
clippy::no_effect,
clippy::unnecessary_operation,
clippy::op_ref,
clippy::trivially_copy_pass_by_ref
clippy::op_ref
)]

#[rustfmt::skip]
Expand Down
34 changes: 17 additions & 17 deletions tests/ui/float_arithmetic.stderr
Original file line number Diff line number Diff line change
@@ -1,103 +1,103 @@
error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:16:5
--> $DIR/float_arithmetic.rs:15:5
|
LL | f * 2.0;
| ^^^^^^^
|
= note: `-D clippy::float-arithmetic` implied by `-D warnings`

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:18:5
--> $DIR/float_arithmetic.rs:17:5
|
LL | 1.0 + f;
| ^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:19:5
--> $DIR/float_arithmetic.rs:18:5
|
LL | f * 2.0;
| ^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:20:5
--> $DIR/float_arithmetic.rs:19:5
|
LL | f / 2.0;
| ^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:21:5
--> $DIR/float_arithmetic.rs:20:5
|
LL | f - 2.0 * 4.2;
| ^^^^^^^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:22:5
--> $DIR/float_arithmetic.rs:21:5
|
LL | -f;
| ^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:24:5
--> $DIR/float_arithmetic.rs:23:5
|
LL | f += 1.0;
| ^^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:25:5
--> $DIR/float_arithmetic.rs:24:5
|
LL | f -= 1.0;
| ^^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:26:5
--> $DIR/float_arithmetic.rs:25:5
|
LL | f *= 2.0;
| ^^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:27:5
--> $DIR/float_arithmetic.rs:26:5
|
LL | f /= 2.0;
| ^^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:33:5
--> $DIR/float_arithmetic.rs:32:5
|
LL | 3.1_f32 + &1.2_f32;
| ^^^^^^^^^^^^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:34:5
--> $DIR/float_arithmetic.rs:33:5
|
LL | &3.4_f32 + 1.5_f32;
| ^^^^^^^^^^^^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:35:5
--> $DIR/float_arithmetic.rs:34:5
|
LL | &3.5_f32 + &1.3_f32;
| ^^^^^^^^^^^^^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:40:5
--> $DIR/float_arithmetic.rs:39:5
|
LL | a + f
| ^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:44:5
--> $DIR/float_arithmetic.rs:43:5
|
LL | f1 + f2
| ^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:48:5
--> $DIR/float_arithmetic.rs:47:5
|
LL | f1 + f2
| ^^^^^^^

error: floating-point arithmetic detected
--> $DIR/float_arithmetic.rs:52:5
--> $DIR/float_arithmetic.rs:51:5
|
LL | (&f1 + &f2)
| ^^^^^^^^^^^
Expand Down
1 change: 0 additions & 1 deletion tests/ui/infinite_iter.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::iter::repeat;
#[allow(clippy::trivially_copy_pass_by_ref)]
fn square_is_lower_64(x: &u32) -> bool {
x * x < 64
}
Expand Down
Loading

0 comments on commit 1e1bd51

Please sign in to comment.