Simplify manual_memcpy suggestion in some cases #3323
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oli-obk
reviewed
Oct 16, 2018
clippy_lints/src/loops.rs
Outdated
| if let BinOpKind::Add = op.node; | ||
| then { | ||
| if let Some(left) = sugg::Sugg::hir_opt(cx, left) { | ||
| if left.to_string() == offset.value { |
There was a problem hiding this comment.
I'm not too happy about the to_string() + comparison.
The other changes in this PR are great, but I'd prefer to postpone this entire if_chain until we have a proper expression minifier/optimizer
There was a problem hiding this comment.
I removed it, but this makes the 0..src.len() check below useless until the expression minifier is implemented. Should I remove that as well?
There was a problem hiding this comment.
Oh... I guess, no need to keep around currently dead code
oli-obk
reviewed
Oct 16, 2018
| let mut dst = [0, 0, 0, 0, 0, 0]; | ||
| let from = 1; | ||
|
|
||
| for i in from..from + src.len() { |
There was a problem hiding this comment.
Please also add a test for values other than src.len(). I think your PR will also improve the suggestion in that case
pengowen123
force-pushed
the
fix_manual_memcpy
branch
from
b7fabb8
to
1b8fa3c
Compare
pengowen123
force-pushed
the
fix_manual_memcpy
branch
from
1b8fa3c
to
8c902d1
Compare
|
@oli-obk Sorry, I forgot to notify you that I removed the dead code. The PR should be good to go unless you have more comments. |
|
Thanks! |
bors
added a commit
that referenced
this pull request
May 2, 2020
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checking whether the entire source slice is copied is difficult, but this should catch common cases.
Fixes #3004