mbox series

[libgpiod,0/4] bindings: rust: clippy: fixed warnings

Message ID 20230629-clippy-v1-0-9ff088713c54@linaro.org
Headers show
Series bindings: rust: clippy: fixed warnings | expand

Message

Erik Schilling June 29, 2023, 11:08 a.m. UTC
This follows up on my promise on Kent's series [1] to look into whether
these casts are needed or not. Most are not, a few are false-positives.

I also explored some shunit2 based test-script to automate the testing,
but that became ugly with linking issue and needs me to revisit it
another time. So this only sends the fixes for now.

[1] https://lore.kernel.org/r/20230612154055.56556-1-warthog618@gmail.com

To: Linux-GPIO <linux-gpio@vger.kernel.org>
Cc: Kent Gibson <warthog618@gmail.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>

Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
---
Erik Schilling (4):
      bindings: rust: clippy: drop unnecessary casts
      bindings: rust: clippy: silence false-positives on casts
      bindings: rust: clippy: drop unneeded conversions
      bindings: rust: clippy: silence false-positive on iterator

 bindings/rust/gpiosim-sys/src/lib.rs         | 2 +-
 bindings/rust/gpiosim-sys/src/sim.rs         | 4 ++--
 bindings/rust/libgpiod/src/chip.rs           | 2 +-
 bindings/rust/libgpiod/src/edge_event.rs     | 2 +-
 bindings/rust/libgpiod/src/event_buffer.rs   | 7 +++++--
 bindings/rust/libgpiod/src/info_event.rs     | 2 +-
 bindings/rust/libgpiod/src/lib.rs            | 2 +-
 bindings/rust/libgpiod/src/line_config.rs    | 4 ++--
 bindings/rust/libgpiod/src/line_info.rs      | 3 +++
 bindings/rust/libgpiod/src/line_request.rs   | 8 ++++----
 bindings/rust/libgpiod/src/line_settings.rs  | 5 ++++-
 bindings/rust/libgpiod/src/request_config.rs | 2 +-
 bindings/rust/libgpiod/tests/chip.rs         | 2 +-
 13 files changed, 27 insertions(+), 18 deletions(-)
---
base-commit: 4510231c95a087f58a155cf74164e403e1e0584f
change-id: 20230629-clippy-890c541c6d09

Best regards,

Comments

Viresh Kumar June 30, 2023, 6:08 a.m. UTC | #1
On 29-06-23, 13:08, Erik Schilling wrote:
> This follows up on my promise on Kent's series [1] to look into whether
> these casts are needed or not. Most are not, a few are false-positives.
> 
> I also explored some shunit2 based test-script to automate the testing,
> but that became ugly with linking issue and needs me to revisit it
> another time. So this only sends the fixes for now.
> 
> [1] https://lore.kernel.org/r/20230612154055.56556-1-warthog618@gmail.com
> 
> To: Linux-GPIO <linux-gpio@vger.kernel.org>
> Cc: Kent Gibson <warthog618@gmail.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Signed-off-by: Erik Schilling <erik.schilling@linaro.org>

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Kent Gibson June 30, 2023, 9:08 a.m. UTC | #2
For future reference, the subject line should be "[libgpiod][PATCH...",
as per the README.
Makes it easier to filter visually, if nothing else.

On Thu, Jun 29, 2023 at 01:09:02PM +0200, Erik Schilling wrote:
> This was fixed, but it is not in stable yet.
> 

This is not a good checkin comment.
State what the problem is and how the patch addresses it.

Same applies to other patches in the series - but I have other comments
on this one, so raising it here.

> Tested build on x86_64, armv7hf, aarch64.
> 

When you say "Tested build", do you mean just compile/clippy, or have you
actually run tests?

Either way, not sure if this should go in the checkin comment - it is
generally implied by the Signed-off that you've tested it to your
satisfaction.
No problem with a more detailed description of how you've tested in
the cover letter.

> Reported-by: Kent Gibson <warthog618@gmail.com>
> Link: https://lore.kernel.org/r/20230612154055.56556-1-warthog618@gmail.com
> Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> ---
>  bindings/rust/libgpiod/src/event_buffer.rs | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/bindings/rust/libgpiod/src/event_buffer.rs b/bindings/rust/libgpiod/src/event_buffer.rs
> index b79e9ea..2e4bfd3 100644
> --- a/bindings/rust/libgpiod/src/event_buffer.rs
> +++ b/bindings/rust/libgpiod/src/event_buffer.rs
> @@ -54,6 +54,9 @@ impl<'a> Iterator for Events<'a> {
>      }
>  
>      fn next(&mut self) -> Option<Self::Item> {
> +        // clippy false-positive, fixed in next clippy release:
> +        // https://github.com/rust-lang/rust-clippy/issues/9820
> +        #[allow(clippy::iter_nth_zero)]
>          self.nth(0)
>      }
>  }
> 

Specify the release in absolute terms, not "next clippy release".

Other than those nits, I'm good with the actual changes in the series
(checked with clippy and running tests on a variety of 32 and 64bit
platforms and compiler versions back to 1.60)

(I am seeing this one test failure on arm32, but that doesn't seem to be related
to this patch:
---- request_config::verify::default stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `OperationFailed(RequestConfigGetConsumer, Errno { code: 2, description: Some("No such file or directory") })`,
 right: `OperationFailed(RequestConfigGetConsumer, Errno { code: 0, description: Some("Success") })`', libgpiod/tests/request_config.rs:18:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Not sure if that is a genuine bug or something funky in my build.)

Cheers,
Kent.
Kent Gibson June 30, 2023, 10:19 a.m. UTC | #3
On Fri, Jun 30, 2023 at 12:05:28PM +0200, Erik Schilling wrote:
> On Fri Jun 30, 2023 at 11:08 AM CEST, Kent Gibson wrote:
> >
> 
> > (I am seeing this one test failure on arm32, but that doesn't seem to be related
> > to this patch:
> > ---- request_config::verify::default stdout ----
> > thread 'main' panicked at 'assertion failed: `(left == right)`
> >   left: `OperationFailed(RequestConfigGetConsumer, Errno { code: 2, description: Some("No such file or directory") })`,
> >  right: `OperationFailed(RequestConfigGetConsumer, Errno { code: 0, description: Some("Success") })`', libgpiod/tests/request_config.rs:18:13
> > note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
> >
> > Not sure if that is a genuine bug or something funky in my build.)
> 
> Is the GPIO_SIM module loaded? Did you test with a custom kernel or
> some distro that ships with it?
> 

That is the only test failing out of the whole suite, so gpiosim is not
the problem.

That is with the latest from the Raspberry Pi rpi-6.4.y branch[1]
(a867309b7a55 so a few days old now), with CONFIG_GPIO_SIM so that I can
run the tests.
Happens to be running on a Pi ZeroW, but I don't think that test is speed
sensitive.  I have done a complete rebuild - same result.

Are there any distos enabling GPIO_SIM yet?

Cheers,
Kent.

[1] https://github.com/raspberrypi/linux
Erik Schilling June 30, 2023, 10:46 a.m. UTC | #4
On Fri Jun 30, 2023 at 12:19 PM CEST, Kent Gibson wrote:
> On Fri, Jun 30, 2023 at 12:05:28PM +0200, Erik Schilling wrote:
> > On Fri Jun 30, 2023 at 11:08 AM CEST, Kent Gibson wrote:
> > > (I am seeing this one test failure on arm32, but that doesn't seem to be related
> > > to this patch:
> > > ---- request_config::verify::default stdout ----
> > > thread 'main' panicked at 'assertion failed: `(left == right)`
> > >   left: `OperationFailed(RequestConfigGetConsumer, Errno { code: 2, description: Some("No such file or directory") })`,
> > >  right: `OperationFailed(RequestConfigGetConsumer, Errno { code: 0, description: Some("Success") })`', libgpiod/tests/request_config.rs:18:13
> > > note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
> > >
> > > Not sure if that is a genuine bug or something funky in my build.)
> > 
> > Is the GPIO_SIM module loaded? Did you test with a custom kernel or
> > some distro that ships with it?
> > 
>
> That is the only test failing out of the whole suite, so gpiosim is not
> the problem.
>
> That is with the latest from the Raspberry Pi rpi-6.4.y branch[1]
> (a867309b7a55 so a few days old now), with CONFIG_GPIO_SIM so that I can
> run the tests.
> Happens to be running on a Pi ZeroW, but I don't think that test is speed
> sensitive.  I have done a complete rebuild - same result.
>
> Are there any distos enabling GPIO_SIM yet?

Fedora does now (after I asked for it [2]). But it does not support any
32-bit ARM targets anymore :/. Can you try reproducing it without the
patches? I would be surprised if this was related to the patches.

I will rebuild my armv7hf VM soon and retry with a self-built kernel.
but not sure when I will get around that.

[2] https://bugzilla.redhat.com/show_bug.cgi?id=2215980

- Erik
Kent Gibson June 30, 2023, 10:50 a.m. UTC | #5
On Fri, Jun 30, 2023 at 12:46:11PM +0200, Erik Schilling wrote:
> On Fri Jun 30, 2023 at 12:19 PM CEST, Kent Gibson wrote:
> > > 
> >
> > That is the only test failing out of the whole suite, so gpiosim is not
> > the problem.
> >
> > That is with the latest from the Raspberry Pi rpi-6.4.y branch[1]
> > (a867309b7a55 so a few days old now), with CONFIG_GPIO_SIM so that I can
> > run the tests.
> > Happens to be running on a Pi ZeroW, but I don't think that test is speed
> > sensitive.  I have done a complete rebuild - same result.
> >
> > Are there any distos enabling GPIO_SIM yet?
> 
> Fedora does now (after I asked for it [2]). But it does not support any
> 32-bit ARM targets anymore :/. Can you try reproducing it without the
> patches? I would be surprised if this was related to the patches.
> 

Tried that - same result with libgpiod master.
So it is not from your patches.

Cheers.
Kent.

> I will rebuild my armv7hf VM soon and retry with a self-built kernel.
> but not sure when I will get around that.
> 
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2215980
> 
> - Erik
Kent Gibson July 1, 2023, 1:10 p.m. UTC | #6
On Fri, Jun 30, 2023 at 12:53:36PM +0200, Erik Schilling wrote:
> On Fri Jun 30, 2023 at 12:50 PM CEST, Kent Gibson wrote:
> > On Fri, Jun 30, 2023 at 12:46:11PM +0200, Erik Schilling wrote:
> > > On Fri Jun 30, 2023 at 12:19 PM CEST, Kent Gibson wrote:
> > > > > 
> > > >
> > > > That is the only test failing out of the whole suite, so gpiosim is not
> > > > the problem.
> > > >
> > > > That is with the latest from the Raspberry Pi rpi-6.4.y branch[1]
> > > > (a867309b7a55 so a few days old now), with CONFIG_GPIO_SIM so that I can
> > > > run the tests.
> > > > Happens to be running on a Pi ZeroW, but I don't think that test is speed
> > > > sensitive.  I have done a complete rebuild - same result.
> > > >
> > > > Are there any distos enabling GPIO_SIM yet?
> > > 
> > > Fedora does now (after I asked for it [2]). But it does not support any
> > > 32-bit ARM targets anymore :/. Can you try reproducing it without the
> > > patches? I would be surprised if this was related to the patches.
> > > 
> >
> > Tried that - same result with libgpiod master.
> > So it is not from your patches.
> 
> Thanks for checking! Will try to reproduce it in the next weeks.
> 

Seems there was something broken in my build.
I did a clean slate rebuild and now all tests are passing.

Cheers,
Kent.