Re: [libgpiod v2][PATCH V8 9/9] bindings: rust: Implement iterator for edge events

[Kent Gibson <warthog618@xxxxxxxxx>]

On Fri, Nov 04, 2022 at 08:33:03PM +0800, Kent Gibson wrote:
> On Fri, Nov 04, 2022 at 05:01:47PM +0530, Viresh Kumar wrote:
> > On 02-11-22, 21:16, Kent Gibson wrote:
> > > It is generally frowned upon to patch your own patch within the same
> > > series, so drop the Suggested-by and squash this into patch 5.
> > > In this instance it is very handy for reviewing though, as the other
> > > patches look pretty clean to me...
> > 
> > I kept it separately for exactly this reason, I was planning to merge it
> > eventually with the other commits.
> > 
> > > > +pub struct Events<'a> {
> > > > +    buffer: &'a mut Buffer,
> > > > +    events: Vec<&'a Event>,
> > > > +    index: usize,
> > > > +}
> > > > +
> > > 
> > > Events is pub but not documented.
> > > 
> > > The events attribute is expensive - requiring a dynamic allocation for
> > > each snapshot.  And you create it unsized and push into it, which could
> > > require resizing and reallocation.
> > > And it is unnecessary - just have Events iterate over the appropriate
> > > indicies and get the ref from the buffer.
> > 
> > I had to do it this way as I wasn't able to get around an error I think. I
> > updated the code now based on suggestions, in my v9 branch, and I get it again:
> > 
> > error: lifetime may not live long enough
> >   --> libgpiod/src/event_buffer.rs:51:9
> >    |
> > 45 | impl<'a> Iterator for Events<'a> {
> >    |      -- lifetime `'a` defined here
> > ...
> > 48 |     fn next(&mut self) -> Option<Self::Item> {
> >    |             - let's call the lifetime of this reference `'1`
> > ...
> > 51 |         event
> >    |         ^^^^^ associated function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`
> > 
> > error: could not compile `libgpiod` due to previous error
> > 
> > I will try to fix this on Monday now, unless you have an answer for me by then
> > :)
> 
> 
>      /// Read an event stored in the buffer.
> -    fn event(&mut self, index: usize) -> Result<&Event> {
> +    fn event<'a>(&mut self, index: usize) -> Result<&'a Event> {
>          if self.events[index].is_null() {
>              // SAFETY: The `gpiod_edge_event` returned by libgpiod is guaranteed to live as long
>              // as the `struct Event`.
> 
> There are still other things that need to be fixed after that, but that
> fixes that particular problem.
> 

I fixed up the other things (handling the iterator returning a Result,
by either unwrapping or flattening as appropriate) in my rust_v9
branch[1].  Also changed Event to a newtype, to remove any possibility
that the struct form could have different memory layout or alignment than
the underlying raw pointer.

That works for me.  Note that the flatten will skip over errors, so
might not be what you want in practice, but my primary interest was to
get everything compiling and the tests passing.

Cheers,
Kent.


[1]https://github.com/warthog618/libgpiod/tree/rust_v9