Fix the bugs of `manual_memcpy`, simplify the suggestion and refactor it
While I’m working on the long procrastinated work to expand `manual_memcpy`(#1670), I found a few minor bugs and probably unidiomatic or old coding style. There is a brief explanation of changes to the behaviour this PR will make below. And, I have a questoin: do I need to add tests for the first and second fixed bugs? I thought it might be too rare cases to include the tests for those. I added for the last one though.
* Bug fix
* It negates resulted offsets (`src/dst_offset`) when `offset` is subtraction by 0. This PR will remove any subtraction by 0 as a part of minification.
```rust
for i in 0..5 {
dst[i - 0] = src[i];
}
```
```diff
warning: it looks like you're manually copying between slices
--> src/main.rs:2:14
|
LL | for i in 0..5 {
- | ^^^^ help: try replacing the loop by: `dst[..-5].clone_from_slice(&src[..5])`
+ | ^^^^ help: try replacing the loop by: `dst[..5].clone_from_slice(&src[..5])`
|
```
* It prints `RangeTo` or `RangeFull` when both of `end` and `offset` are 0, which have different meaning. This PR will print 0. I could reject the cases `end` is 0, but I thought I won’t catch other cases `reverse_range_loop` will trigger, and it’s over to catch every such cases.
```rust
for i in 0..0 {
dst[i] = src[i];
}
```
```diff
warning: it looks like you're manually copying between slices
--> src/main.rs:2:14
|
LL | for i in 0..0 {
- | ^^^^ help: try replacing the loop by: `dst.clone_from_slice(&src[..])`
+ | ^^^^ help: try replacing the loop by: `dst[..0].clone_from_slice(&src[..0])`
|
```
* it prints four dots when `end` is `None`. This PR will ignore any `for` loops without `end` because a `for` loop that takes `RangeFrom` as its argument and contains indexing without the statements or the expressions that end loops such as `break` will definitely panic, and `manual_memcpy` should ignore the loops with such control flow.
```rust
fn manual_copy(src: &[u32], dst: &mut [u32]) {
for i in 0.. {
dst[i] = src[i];
}
}
```
```diff
-warning: it looks like you're manually copying between slices
- --> src/main.rs:2:14
- |
-LL | for i in 0.. {
- | ^^^ help: try replacing the loop by: `dst[....].clone_from_slice(&src[....])`
- |
```
* Simplification of the suggestion
* It prints 0 when `start` or `end` and `offset` are same (from #3323). This PR will use `RangeTo`
changelog: fixed the bugs of `manual_memcpy` and also simplify the suggestion.
New lint `match_vec_item`
Added new lint to warn a match on index item which can panic. It's always better to use `get(..)` instead.
Closes#5500
changelog: New lint `match_on_vec_items`
- Show just one error message with multiple suggestions in case of
using multiple times an OS in target family position
- Only suggest #[cfg(unix)] when the OS is in the Unix family
- Test all the operating systems
Don't trigger while_let_on_iterator when the iterator is recreated every iteration
r? @phansch
Fixes#1654
changelog: Fix false positive in [`while_let_on_iterator`]
Downgrade match_bool to pedantic
I don't quite buy the justification in https://rust-lang.github.io/rust-clippy/. The justification is:
> It makes the code less readable.
In the Rust codebases I've worked in, I have found people were comfortable using `match bool` (selectively) to make code more readable. For example, initializing struct fields is a place where the indentation of `match` can work better than the indentation of `if`:
```rust
let _ = Struct {
v: {
...
},
w: match doing_w {
true => ...,
false => ...,
},
x: Nested {
c: ...,
b: ...,
a: ...,
},
y: if doing_y {
...
} else { // :(
...
},
z: ...,
};
```
Or sometimes people prefer something a bit less pithy than `if` when the meaning of the bool doesn't read off clearly from the condition:
```rust
if set.insert(...) {
... // ???
} else {
...
}
match set.insert(...) {
// set.insert returns false if already present
false => ...,
true => ...,
}
```
Or `match` can be a better fit when the bool is playing the role more of a value than a branch condition:
```rust
impl ErrorCodes {
pub fn from(b: bool) -> Self {
match b {
true => ErrorCodes::Yes,
false => ErrorCodes::No,
}
}
}
```
And then there's plain old it's-1-line-shorter, which means we get 25% more content on a screen when stacking a sequence of conditions:
```rust
let old_noun = match old_binding.is_import() {
true => "import",
false => "definition",
};
let new_participle = match new_binding.is_import() {
true => "imported",
false => "defined",
};
```
Bottom line is I think this lint fits the bill better as a pedantic lint; I don't think linting on this by default is justified.
changelog: Remove match_bool from default set of enabled lints
