Clear Clippy warnings #35

tcharding · 2021-02-10T06:30:08Z

This PR clears all Clippy warnings (incl. nightly) except for documentation of unsafe

warning: unsafe function's docs miss `# Safety` section

Please note, the first patch runs cargo fmt if that is not wanted I can remove it.

Commit d7f6956 Use std::mem::MaybeUninit needs special attention please.

This is kinda a big PR for my first one so if you'd like me to split the more trivial patches out into a separate PR I can do that also.

Thanks

andersk · 2021-02-10T07:08:29Z

src/mpq.rs

@@ -70,17 +71,9 @@ impl Mpq {
        &mut self.mpq
    }

-    pub fn new() -> Mpq {


We can’t go around deleting parts of the public API just because Clippy suggests it…

Oh, very good point. My bad.

Will fix and re-spin tomorrow. Thanks for looking at this.

andersk · 2021-02-10T07:18:13Z

You claim that the first commit is cargo fmt, but in reality it only reformats three files, with four other files getting reformatted in various other commits. This makes it difficult to see what those commits actually changed.

tcharding · 2021-02-10T07:21:43Z

You claim that the first commit is cargo fmt, but in reality it only reformats three files, with four other files getting reformatted in various other commits. This makes it difficult to see what those commits actually changed.

Oh boy, sloppy PR huh. I moved that commit to the front of the list and didn't look through the first three diffs, my bad (again). Feel free to ignore me for the rest of the day, I'll make a better effort tomorrow.

tcharding · 2021-02-11T00:04:38Z

Implemented review suggestions:

Fixed all cargo fmt changes to be in the first patch.
Removed default -> new change

Question, I'm not totally sure the Error change is not a breaking API change. I think it is but am not sure. Please review 43cee7b Implement Display for error types with this in mind.

Thanks.

andersk · 2021-02-11T02:11:17Z

Fixed all cargo fmt changes to be in the first patch.

Despite your claim, this is not fixed.

andersk · 2021-02-11T02:16:07Z

src/mpq.rs

-    }
-}
-
-impl Error for ParseMpqError {


You have removed these impls completely, which is neither desirable nor backwards-compatible. The soft-deprecation of Error::description means you can use an empty impl, not no impl.

Oh cool, thanks for the info. I will fix and re-spin.

tcharding · 2021-02-11T05:03:21Z

Fixed all cargo fmt changes to be in the first patch.

Despite your claim, this is not fixed.

Woah, bad vodoo on this PR. I was sure I went through the whole patch set but seems I don't know how to read a diff.

Clippy emits: warning: trait objects without an explicit `dyn` are deprecated Use dyn as suggested.

Clippy emits: warning: redundant field names in struct initialization As suggested, remove the redundant field names.

`unititialized` is deprecated. Use the newer `MaybeUninit`.

Error trait has changed a bit since Rust v1.30.0 we can implement `Display` and remove the deprecated `description`. Also `cause` has a default implementation that returns `None` so removing the custom one makes no logic change.

Clippy emits: warning: manual `RangeInclusive::contains` implementation As suggested, we can use a range and `.contains`.

We have a few places where we manually switch on conditionals, we can use `std::cmp` for this. Found by Clippy.

tcharding · 2021-02-12T01:59:35Z

3rd time lucky. This time I've actually put all the fmt changes in the first patch. Also split out a separate patch for the popcount unit test fix aa23b5d Assert in popcount test (found by @mikelodder7 in his PR). Finally I added back in the empty 'impl Error' blocks as required.

Thanks for your patience.

mikelodder7 · 2021-02-12T04:20:02Z

Awesome

andersk · 2021-02-12T04:19:26Z

src/mpz.rs

            let mut first_nul = None;
-            let mut index : usize = 0;
-            for elem in &vector {
+            for (index, elem) in vector.iter().enumerate() {
                if *elem == 0 {
                    first_nul = Some(index);
                    break;
                }
-                index += 1;
            }


Could be simplified to

let first_nul = vector.iter().position(|elem| *elem == 0);

Added the .unwrap_or onto it as well :)

andersk · 2021-02-12T04:41:48Z

src/test.rs

    }

    #[test]
    fn test_to_str_radix() {
        let x: Mpz = From::<i64>::from(255);
-        assert!(x.to_str_radix(16) == "ff".to_string());
+        assert!(x.to_str_radix(16) == *"ff");


The * isn’t needed; simply assert!(x.to_str_radix(16) == "ff") works fine (or assert_eq!).

Cool, this surprised me, I didn't think assert_eq! could compare an &str with a String. Thanks, implemented as suggested. I added it as a new patch on top of this branch to make re-review easier. I can squash it with the original one if you prefer.

Rust has a boatload of PartialEq impls, including PartialEq<str> for str, PartialEq<String> for String, PartialEq<str> for String, PartialEq<String> for str, PartialEq<&'a str> for String, PartialEq<String> for &'a str.

You haven’t actually pushed anything though. I’m not the maintainer, but for my part I’m fine with squashing.

Squashed and force pushed. Thanks for the tip about PartialEq implementations!

Instead of manually iterating for the first nul character we can use the `position` combinator. This reduces lines of code considerably.

The only reason we have the function return and match statement is to give meaning to the TODO. Clippy does not like this code, suggests use of `matches!` macro. Instead, we can explain the TODO more fully in text and remove the code.

Clippy emits: warning: `0 as *mut _` detected As suggested, use `std::ptr::null_mut::<size_t>()` instead.

Clippy emits: warning: unneeded `return` statement As suggested, do not use explicit return statement.

Clippy complains a bunch about various things like warning: statement can be reduced This is caused by unusual code in unit tests because we are intentionally testing the operator implementations. These warnings can be cleared by ignoring the return value.

The current test of `popcount` is not doing anything. Add an assertion on the expected return value.

We can take advantage of Rust's `PartialEq<str> for String` implementation when doing test assertions.

We are testing the operator implementation, instruct Clippy to ignore the `eq_op` lint.

tcharding · 2021-05-06T23:11:45Z

Any progress on this one please? I wouldn't normally ask but quite a bit of effort was supplied by @andersk during review it would be nice to see this one go in.

tcharding · 2022-01-19T03:49:56Z

This has been open for almost a year now, closing.

andersk reviewed Feb 10, 2021

View reviewed changes

tcharding changed the title ~~Clear cippy warnings~~ Clear Clippy warnings Feb 10, 2021

andersk reviewed Feb 11, 2021

View reviewed changes

tcharding added 7 commits February 12, 2021 12:45

Run cargo fmt

b646a0c

Use dyn

c95bdb9

Clippy emits: warning: trait objects without an explicit `dyn` are deprecated Use dyn as suggested.

Remove redundant field names

ddc4c63

Clippy emits: warning: redundant field names in struct initialization As suggested, remove the redundant field names.

Use std::mem::MaybeUninit

5fc98e3

`unititialized` is deprecated. Use the newer `MaybeUninit`.

Implement Display for error types

e4da7a5

Error trait has changed a bit since Rust v1.30.0 we can implement `Display` and remove the deprecated `description`. Also `cause` has a default implementation that returns `None` so removing the custom one makes no logic change.

Use range and contains

0205e11

Clippy emits: warning: manual `RangeInclusive::contains` implementation As suggested, we can use a range and `.contains`.

Fully utilise std::cmd

b3dbe23

We have a few places where we manually switch on conditionals, we can use `std::cmp` for this. Found by Clippy.

andersk reviewed Feb 12, 2021

View reviewed changes

tcharding added 8 commits February 15, 2021 19:17

Use position combinator

bf5b415

Instead of manually iterating for the first nul character we can use the `position` combinator. This reduces lines of code considerably.

Ignore function return with comment

b758c8d

The only reason we have the function return and match statement is to give meaning to the TODO. Clippy does not like this code, suggests use of `matches!` macro. Instead, we can explain the TODO more fully in text and remove the code.

Use ptr::null_mut()

f36630f

Clippy emits: warning: `0 as *mut _` detected As suggested, use `std::ptr::null_mut::<size_t>()` instead.

Do not use explicit return statement

364e7de

Clippy emits: warning: unneeded `return` statement As suggested, do not use explicit return statement.

Ignore return values

9383851

Clippy complains a bunch about various things like warning: statement can be reduced This is caused by unusual code in unit tests because we are intentionally testing the operator implementations. These warnings can be cleared by ignoring the return value.

Assert in popcount test

fd5e798

The current test of `popcount` is not doing anything. Add an assertion on the expected return value.

Use static string in assertions

3713016

We can take advantage of Rust's `PartialEq<str> for String` implementation when doing test assertions.

Allow eq_op

b1fe1cf

We are testing the operator implementation, instruct Clippy to ignore the `eq_op` lint.

tcharding closed this Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear Clippy warnings #35

Clear Clippy warnings #35

tcharding commented Feb 10, 2021 •

edited

Loading

andersk Feb 10, 2021

tcharding Feb 10, 2021

tcharding Feb 10, 2021

andersk commented Feb 10, 2021

tcharding commented Feb 10, 2021

tcharding commented Feb 11, 2021

andersk commented Feb 11, 2021

andersk Feb 11, 2021

tcharding Feb 11, 2021

tcharding commented Feb 11, 2021

tcharding commented Feb 12, 2021 •

edited

Loading

mikelodder7 commented Feb 12, 2021

andersk Feb 12, 2021

tcharding Feb 13, 2021

andersk Feb 12, 2021

tcharding Feb 13, 2021

andersk Feb 13, 2021

tcharding Feb 15, 2021

tcharding commented May 6, 2021

tcharding commented Jan 19, 2022 •

edited

Loading

Clear Clippy warnings #35

Clear Clippy warnings #35

Conversation

tcharding commented Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersk commented Feb 10, 2021

tcharding commented Feb 10, 2021

tcharding commented Feb 11, 2021

andersk commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Feb 11, 2021

tcharding commented Feb 12, 2021 • edited Loading

mikelodder7 commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented May 6, 2021

tcharding commented Jan 19, 2022 • edited Loading

tcharding commented Feb 10, 2021 •

edited

Loading

tcharding commented Feb 12, 2021 •

edited

Loading

tcharding commented Jan 19, 2022 •

edited

Loading