Some Clippy lints

[max-sixty]

I'm trying to get familiar with the repo and thought this was a reasonable way to start.

It's not complete - there are a lot of immediately dereferencing a reference around s! invocations (some I explicitly ignored), and some around using assert_eq on f32 & f64.

It's not yet possible to set Clippy rules for a whole workspace - it's required to do crate-by-crate - so there are some standard ignore pieces copied and pasted. I'm not sure how to do this for the test path which don't have a lib.rs or mod.rs.

[max-sixty]

Are the performance issues solved now? This comment was from several years ago

[jturner314]

The commit adding the comment (86415f7) says

Use counted loop in unordered_foreach_mut

This improves the generated code for the inner loop we use for
.assign_scalar() and other array operations with a single scalar.

The old code would generate something that looked like it assumed the
element being written to and the iterator's pointer could alias another. This
new version is unrolled and vectorized to a larger degree, but it does
still not produce memset for some inputs like rust could do in january 2016.

name before ns/iter after ns/iter diff ns/iter
diff %
assign_scalar_2d_corder 1,631 868 -763 -46.78%
assign_scalar_2d_cutout 1,704 1,332 -372 -21.83%
assign_scalar_2d_forder 1,628 869 -759 -46.62%
assign_zero_2d_corder 1,602 855 -747 -46.63%
assign_zero_2d_cutout 1,703 1,335 -368 -21.61%
assign_zero_2d_forder 1,618 858 -760 -46.97%

before: Using the slice iterator
after: using indexed loop.

I tried switching the implementation to use for_each and got the same results for these benchmarks as the range-based implementation, so I think that would work well. (I didn't try a for loop.)

        if let Some(slc) = self.as_slice_memory_order_mut() {
            slc.iter_mut().for_each(f);
        } else {
            for row in self.inner_rows_mut() {
                row.into_iter_().fold((), |(), elt| f(elt));
            }
        }

(Fwiw, I tried switching the .fold to .for_each too, but that hurt performance on the *_cutout benchmarks. That may be worth looking into. (Do we need to implement for_each for IterMut instead of relying on the default implementation?))

[max-sixty]

I've ended up pushing through and either correcting them all or ignoring them.

A couple I need help on - they're 'allowed' for the moment, but my rust understanding wasn't enough to solve the issue in a reasonable way.
And a few need a review - I'm sure my solutions won't be optimal is some of the cases.

[max-sixty]

Hi team - any thoughts here? Would be great to get some of this merged before we start getting conflicts

Happy to split into an 'easy' PR and one that requires more discussion if anything requires that

[max-sixty]

Yes @alex179ohm - I think those are covered by this PR, is that correct?

[alex179ohm]

@max-sixty, no
Unfortunately, the nice-semver error still remain until the 0.13 will be released

[max-sixty]

Unfortunately, the nice-semver error still remain until the 0.13 will be released

I think this wants a semver version, so 0.13.0, not necessarily a released version. That change is in this PR

[max-sixty]

If this is still an open issue, shall I remove that change so we can merge? I'm cognizant that this touches a lot of code and keen to avoid merge conflicts

[alex179ohm]

@max-sixty All clippy warnings are fixed on your clippy branch? If so it could be fine add clippy on the CI/travis build pipeline

[max-sixty]

If so it could be fine add clippy on the CI/travis build pipeline

Ask and ye shall receive: https://github.com/rust-ndarray/ndarray/pull/642/files?file-filters%5B%5D=.sh#diff-407dae7942292c08ef6b0f9b2b8c2677R17

[LukeMathWalker]

I agree that this PR touches a lot of code and it would be better to get it merged sooner rather than later (even more so because several people try to have a go at it, and we end up with duplicate effort).
I know @jturner314 cannot put much time in ndarray right now, so I would propose to set aside #642 (comment) until we have time to go back at it. You can use a deny statement to make the linting pass, so that we can merge the rest. What do you think @max-sixty?

[max-sixty]

Great, done @LukeMathWalker

[LukeMathWalker]

Merged @max-sixty 👍 Can you open an issue to track the remaining lints?
@alex179ohm - should I close the other PR or are there additional changes there that we need to review?

[max-sixty]

Great - @LukeMathWalker - here: #657

Let me know if I missed anything