Skip to content

Commit 056716e

Browse files
authored
clippy: remove needless borrows (#1283)
This was made an error in Clippy recently, so we'll need to fix it if we want CI to work. According to https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow What it does: Checks for address of operations (`&`) that are going to be dereferenced immediately by the compiler. Why is this bad: Suggests that the receiver of the expression borrows the expression. Commentary as it applies to the Rust track: I can get behind the idea that it's better not to mislead the reader of the code about how many levels of reference the called function needs. Most of these stem from a misunderstanding of what expressions were already borrowed. They are listed below. Note carefully inconsistencies introduced in sublist and tournament, and consider whether we have a solution for this inconsistency before approving. The concern would be any confusion it may cause to those learning Rust. It is consistent from the standpoint of the type we're ultimately passing, but it's inconsistent from the standpoint of what the call sites look like. * Dominoes: The input is `&[Domino]`, and check takes `&[Domino]`. calling `check(&input)` passes `&&[Domino]` whereas we should call `check(input)` to pass `&[Domino]` * DOT DSL: `iter` iterates over `&T`. Therefore the `name` is a `&&str` (recall that string literals in Rust produce `&str`) and we are iterating over a `[&str]` producing `&&str`; since `Node::new` takes `&str`, just `name` would be better than `&name`, which would pass `&&&str` * grep: The patterns are string literals such as `"Agamemnon"`. Recall that string literals in Rust produce `&str`, and grep takes `&str`, so no additional `&` should be added. In fact, we should do this for the macro too, even though Clippy does not seem to catch those. * pangram: Recall that string literals in Rust produce `&str`, and is_pangram takes `&str`, so no additional `&` should be added. * sublist: The `v` was declared as `&[u32]`, and `sublist` takes two `&[T]`, so no additional `&` should be added. Inconsistency introduced: `sublist` is called with `&` in all other instances in this file because they are either slice literals or Vec, which do require the `&`. * tournament: Recall that string literals in Rust produce `&str`, and `tally` takes `&str`, so no additional `&` should be added. Inconsistency introduced: The first four cases use a string literal and thus do not require `&`. The next cases use `String` (recall that we decided that this was the best way to show the multi-line strings in #152 (comment)), and therefore require `&`. Consider carefully whether this may be confusing to students that some require `&` and some not.
1 parent 8d6859a commit 056716e

File tree

6 files changed

+21
-21
lines changed

6 files changed

+21
-21
lines changed

exercises/practice/dominoes/tests/dominoes.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ fn check(input: &[Domino]) -> CheckResult {
6565
}
6666

6767
fn assert_correct(input: &[Domino]) {
68-
match check(&input) {
68+
match check(input) {
6969
Correct => (),
7070
GotInvalid => panic!("Unexpectedly got invalid on input {:?}", input),
7171
ChainingFailure(output) => panic!(

exercises/practice/dot-dsl/tests/dot-dsl.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ fn test_graph_stores_attributes() {
130130
&["a", "b", "c"]
131131
.iter()
132132
.zip(attributes.iter())
133-
.map(|(name, &attr)| Node::new(&name).with_attrs(&[attr]))
133+
.map(|(name, &attr)| Node::new(name).with_attrs(&[attr]))
134134
.collect::<Vec<_>>(),
135135
);
136136

exercises/practice/grep/tests/grep.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ macro_rules! set_up_test_case {
127127

128128
let expected = vec![$($expected),*];
129129

130-
process_grep_case(&pattern, &flags, &files, &expected);
130+
process_grep_case(pattern, &flags, &files, &expected);
131131
}
132132
};
133133
($(#[$flag:meta])+ $test_case_name:ident(pattern=$pattern:expr, flags=[$($grep_flag:expr),*], files=[$($file:expr),+], prefix_expected=[$($expected:expr),*])) => {
@@ -141,7 +141,7 @@ macro_rules! set_up_test_case {
141141

142142
let expected = vec![$(concat!(stringify!($test_case_name), "_", $expected)),*];
143143

144-
process_grep_case(&pattern, &flags, &files, &expected);
144+
process_grep_case(pattern, &flags, &files, &expected);
145145
}
146146

147147
}
@@ -169,7 +169,7 @@ fn test_nonexistent_file_returns_error() {
169169

170170
let files = vec!["test_nonexistent_file_returns_error_iliad.txt"];
171171

172-
assert!(grep(&pattern, &flags, &files).is_err());
172+
assert!(grep(pattern, &flags, &files).is_err());
173173
}
174174

175175
#[test]
@@ -185,7 +185,7 @@ fn test_grep_returns_result() {
185185

186186
test_fixture.set_up();
187187

188-
assert!(grep(&pattern, &flags, &files).is_ok());
188+
assert!(grep(pattern, &flags, &files).is_ok());
189189
}
190190

191191
// Test grepping a single file

exercises/practice/pangram/tests/pangram.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,68 +3,68 @@ use pangram::*;
33
#[test]
44
fn empty_strings_are_not_pangrams() {
55
let sentence = "";
6-
assert!(!is_pangram(&sentence));
6+
assert!(!is_pangram(sentence));
77
}
88

99
#[test]
1010
#[ignore]
1111
fn classic_pangram_is_a_pangram() {
1212
let sentence = "the quick brown fox jumps over the lazy dog";
13-
assert!(is_pangram(&sentence));
13+
assert!(is_pangram(sentence));
1414
}
1515

1616
#[test]
1717
#[ignore]
1818
fn pangrams_must_have_all_letters() {
1919
let sentence = "a quick movement of the enemy will jeopardize five gunboats";
20-
assert!(!is_pangram(&sentence));
20+
assert!(!is_pangram(sentence));
2121
}
2222

2323
#[test]
2424
#[ignore]
2525
fn pangrams_must_have_all_letters_two() {
2626
let sentence = "the quick brown fish jumps over the lazy dog";
27-
assert!(!is_pangram(&sentence));
27+
assert!(!is_pangram(sentence));
2828
}
2929

3030
#[test]
3131
#[ignore]
3232
fn pangrams_must_include_z() {
3333
let sentence = "the quick brown fox jumps over the lay dog";
34-
assert!(!is_pangram(&sentence));
34+
assert!(!is_pangram(sentence));
3535
}
3636

3737
#[test]
3838
#[ignore]
3939
fn underscores_do_not_affect_pangrams() {
4040
let sentence = "the_quick_brown_fox_jumps_over_the_lazy_dog";
41-
assert!(is_pangram(&sentence));
41+
assert!(is_pangram(sentence));
4242
}
4343

4444
#[test]
4545
#[ignore]
4646
fn numbers_do_not_affect_pangrams() {
4747
let sentence = "the 1 quick brown fox jumps over the 2 lazy dogs";
48-
assert!(is_pangram(&sentence));
48+
assert!(is_pangram(sentence));
4949
}
5050

5151
#[test]
5252
#[ignore]
5353
fn numbers_can_not_replace_letters() {
5454
let sentence = "7h3 qu1ck brown fox jumps ov3r 7h3 lazy dog";
55-
assert!(!is_pangram(&sentence));
55+
assert!(!is_pangram(sentence));
5656
}
5757

5858
#[test]
5959
#[ignore]
6060
fn capitals_and_punctuation_can_be_in_pangrams() {
6161
let sentence = "\"Five quacking Zephyrs jolt my wax bed.\"";
62-
assert!(is_pangram(&sentence));
62+
assert!(is_pangram(sentence));
6363
}
6464

6565
#[test]
6666
#[ignore]
6767
fn non_ascii_characters_can_be_in_pangrams() {
6868
let sentence = "Victor jagt zwölf Boxkämpfer quer über den großen Sylter Deich.";
69-
assert!(is_pangram(&sentence));
69+
assert!(is_pangram(sentence));
7070
}

exercises/practice/sublist/tests/sublist.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use sublist::{sublist, Comparison};
44
fn empty_equals_empty() {
55
let v: &[u32] = &[];
66

7-
assert_eq!(Comparison::Equal, sublist(&v, &v));
7+
assert_eq!(Comparison::Equal, sublist(v, v));
88
}
99

1010
#[test]

exercises/practice/tournament/tests/tournament.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ fn just_the_header_if_no_input() {
33
let input = "";
44
let expected = "Team | MP | W | D | L | P";
55

6-
assert_eq!(tournament::tally(&input), expected);
6+
assert_eq!(tournament::tally(input), expected);
77
}
88

99
#[test]
@@ -15,7 +15,7 @@ fn a_win_is_three_points_a_loss_is_zero_points() {
1515
+ "Allegoric Alaskans | 1 | 1 | 0 | 0 | 3\n"
1616
+ "Blithering Badgers | 1 | 0 | 0 | 1 | 0";
1717

18-
assert_eq!(tournament::tally(&input), expected);
18+
assert_eq!(tournament::tally(input), expected);
1919
}
2020

2121
#[test]
@@ -27,7 +27,7 @@ fn a_win_can_also_be_expressed_as_a_loss() {
2727
+ "Allegoric Alaskans | 1 | 1 | 0 | 0 | 3\n"
2828
+ "Blithering Badgers | 1 | 0 | 0 | 1 | 0";
2929

30-
assert_eq!(tournament::tally(&input), expected);
30+
assert_eq!(tournament::tally(input), expected);
3131
}
3232

3333
#[test]
@@ -39,7 +39,7 @@ fn a_different_team_can_win() {
3939
+ "Blithering Badgers | 1 | 1 | 0 | 0 | 3\n"
4040
+ "Allegoric Alaskans | 1 | 0 | 0 | 1 | 0";
4141

42-
assert_eq!(tournament::tally(&input), expected);
42+
assert_eq!(tournament::tally(input), expected);
4343
}
4444

4545
#[test]

0 commit comments

Comments
 (0)