Re: [PATCH libgpiod 4/4] bindings: rust: clippy: silence false-positive on iterator

["Erik Schilling" <erik.schilling@linaro.org>]

On Fri Jun 30, 2023 at 11:08 AM CEST, Kent Gibson wrote:
>
> For future reference, the subject line should be "[libgpiod][PATCH...",
> as per the README.
> Makes it easier to filter visually, if nothing else.

I am using b4 to send the patches (otherwise I would screw up this stuff
even more) which always merges the prefixes together. Raised the point
there:

https://lore.kernel.org/r/CTPW75Q2ISUN.251ELTNP6RB22@fedora

> 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.

Will rephrase it in next version.

> 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?

This is a bit complex... Originally I intended to send this along some
shunit2 script that checks build tests and clippy lints. But that became
a bit more involving than I hoped... By defauly we link gpiosim into the
tests statically and the static lib is not built with PIC by default.

Rust, however, by default wants to build the tests with PIE. For some
reason that works on Debian, but not on Fedora. It also broke down on
Debian on armv7hf. So I was not able to come up with a configuration
set that worked perfectly in all situations (apart from building the
C lib with PIC) and decided not to send scripts to automate the check
(for now).

After wasting way too much time on this, I then realized that I was
missing the GPIO_SIM module and skipped the tests for armv7hf since I
dropped the check script from this series anyway.

TLDR: I tested build AND cargo test on x86_64 and aarch64, but only the
build with armv7hf (I built test and examples though).

> 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.

Right... Since I dropped my script idea how now I can move this to the
cover, will expand what I tested in v2.

> > 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".

I cannot tell the version of the next release that will include it since
it is not released yet, but I can state the clippy version that caused
the error.

> 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)

Thanks for testing!

> (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?

- Erik