Skip to content

Commit

Permalink
Auto merge of #5536 - rail-rain:fix_manual_memcpy, r=phansch
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bors committed May 2, 2020
2 parents d270887 + 461f4a3 commit e3b8c41
Show file tree
Hide file tree
Showing 3 changed files with 207 additions and 189 deletions.
Loading

0 comments on commit e3b8c41

Please sign in to comment.