Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clippy warnings. #605

Closed
wants to merge 1 commit into from
Closed

Fix clippy warnings. #605

wants to merge 1 commit into from

Conversation

dineshadepu
Copy link

The following warnings are omitted

  1. Redundant field names
  2. Constants by default have a 'static' life time
  3. Redundant pattern matching
  4. match statements by dereferencing
  5. Using clone on copy type
  6. Inclusive ranges
  7. Manual copying loops
  8. Dedicated clone methods

The following warnings are omitted
1. Redundant field names
2. Constants by default have a `'static'` life time
3. Redundant pattern matching
4. match statements by dereferencing
5. Using clone on copy type
6. Inclusive ranges
7. Manual copying loops
8. Dedicated clone methods
@dineshadepu
Copy link
Author

There are some warnings such as

warning: the loop variable `i` is only used to index `slc`.
    --> src/lib.rs:1340:22
     |
1340 |             for i in 0..slc.len() {
     |                      ^^^^^^^^^^^^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop
help: consider using an iterator
     |
1340 |             for <item> in &mut slc {
     |                 ^^^^^^    ^^^^^^^^

I tried to solve this, but I get into copy or clone not implemented errors.

And I also didn't change the fields with single char. The warning looks like

warning: 6th binding whose name is just one char
   --> src/linalg/impl_linalg.rs:508:17
    |
508 |         let mut j = 0;

I felt it is meaning full.

And also I haven't fixed the specific error catching, the warning looks like

warning: Err(_) will match all errors, maybe not a good idea
   --> src/impl_constructors.rs:140:13
    |
140 |             Err(_) => panic!(
    |             ^^^^^^
...
243 |         let _ = size_of_shape_checked_unwrap!(&shape.dim);
    |                 ----------------------------------------- in this macro invocation
    |
    = note: to remove this warning, match each error separately or use unreachable macro
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_wild_err_arm

I am looking into the code, once I get the error handling and error types defined in ndarray I will try to change that to specific error types if needed.

@LukeMathWalker
Copy link
Member

Thanks for working on this @dineshadepu!
Let us know when you think it's a good time to review this PR 💪

@dineshadepu
Copy link
Author

Yes, it ready to review.

@LukeMathWalker

@LukeMathWalker LukeMathWalker mentioned this pull request Mar 26, 2019
31 tasks
@LukeMathWalker
Copy link
Member

I'd like to have clippy in the CI pipeline: it would be nice to have a reference that every contributor on a PR can look at to see linter warnings. Ideally the build should only fail on errors, not warnings (let's start with a bit of slack).
I have cloned your branch locally @dineshadepu and I can see a bunch of clippy errors:

  • this loop never actually loops
  • you are implementing Hash explicitly but have derived PartialEq
  • you are deriving Hash but have implemented PartialEq explicitly
  • use of #[inline] on trait method contiguous_stride which has no body
  • the since field must contain a semver-compliant version

To add Clippy to our CI pipeline, we should either resolve these errors or disable Clippy for those error classes/functions/methods.
Similarly, if there are warnings that we do not plan to address, we should disable them, either locally or globally.

@dineshadepu dineshadepu closed this Apr 8, 2019
@alex179ohm
Copy link

I made some pull-requests on these errors but they are all referenced to the #642 issue (I just looked for clippy on issue's search bar and the 642 has been the first open issue coming out :))
Maybe the coming pull-requests should be referenced to this issue?
there is just one error unsolved and it is the semver-compliant related to the since field (does the 0.13 release exist?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants