explicit_counter_loop improvement suggestion

[martinlindhe]

Hello! I'm just learning rust and clippy has been a valuable resource for improving my code, thanks!
Today I got confused by one of the clippy suggestions.
I had a look at the source but not sure how to implement proposed improvements.

Consider the following

fn load_rom(&mut self, data: &[u8]) {
    let min = self.get_offset();
    let max = min + data.len();

    let mut rom_pos = 0;
    for i in min..max {
        self.memory[i] = data[rom_pos];
        rom_pos += 1;
    }
}

clippy gives the following warning:

warning: the variable `rom_pos` is used as a loop counter. Consider using `for (rom_pos, item) in min..max.enumerate()` or similar iterators
   --> src/cpu.rs:546:9
    |
546 |           for i in min..max {
    |  _________^ starting here...
547 | |             self.memory[i] = data[rom_pos];
548 | |             rom_pos += 1;
549 | |         }
    | |_________^ ...ending here
    |
    = note: #[warn(explicit_counter_loop)] on by default
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop

attempting to rework the code as suggested:

for (rom_pos, i) in min..max.enumerate() {
    self.memory[i] = data[rom_pos];
}

... does not compile:

error: no method named `enumerate` found for type `usize` in the current scope
   --> src/cpu.rs:552:38
    |
552 |         for (rom_pos, i) in min..max.enumerate() {
    |                                      ^^^^^^^^^
    |
    = note: the method `enumerate` exists but the following trait bounds were not satisfied: `usize : std::iter::Iterator`

error[E0308]: mismatched types
   --> src/cpu.rs:552:13
    |
552 |         for (rom_pos, i) in min..max.enumerate() {
    |             ^^^^^^^^^^^^ expected usize, found tuple
    |
    = note: expected type `usize`
               found type `(_, _)`

Eventually I realized I just need to wrap the min..max in parenthesis to solve it.

I feel an improved error message in check_for_loop_explicit_counter could look like this:

warning: the variable `rom_pos` is used as a loop counter.
Consider using `for (rom_pos, i) in (min..max).enumerate()`

Also it hardcodes "item" rather than using the variable name "i"

[oli-obk]

A further suggestion:

self.memory[min..max].copy_from_slice(data)

[martinlindhe]

Even better, thanks for the suggestion!

[rail-rain]

Hello.

I would like to try this as my first contribution.

But there are three issues:

1. explicit_counter_loop suggestion cause error if for loop's argument precedence over method call

2. Also it hardcodes "item" rather than using the variable name "i"

3. A further suggestion:

   self.memory[min..max].copy_from_slice(data)

Should I separate PR?

[flip1995]

The first step would be to address 1. and 2. This should be done in one PR. You have to turn this lint into a span_lint_and_sugg here:
https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/loops.rs#L1492-L1502

You can take a look at clippy_lints/src/utils/sugg.rs for the part of the parentheses.

3. is really specific for this use case, where you just copy from one slice into another and don't do anything else in the for loop. This should be addressed separatly (maybe even a new lint?).

If you have any questions or need help with something, feel free to ask here or open a WIP PR.

[rail-rain]

Thank you. I'll make PR for 1. and 2.

[rail-rain]

Since 1. and 2. is addressed by #3901, only 3. remains. Should we close this issue (and open new issue for 3.)? or keep open until 3. is implemented?

(on a lighter note, I think 3. is manual_memcpy)

[rail-rain]

Never mind. I started to work on a PR that addresses 3. as manual_memcpy.