Re: [libgpiod] Rethinking struct gpiod_line_bulk

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Oct 20, 2020 at 5:06 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>

[snip]

> > >
> > > I don't have a problem with that definition, appropriately documented,
> > > but I wouldn't want to expose that to the user, so my preference would
> > > be to make it opaque, but....
> > >
> > > ... that will make the API more awkward to use, as the methods that
> > > populate a bulk will now have to have that bulk constructed beforehand.
> > > Or you change them to accept a struct gpiod_line_bulk ** to return the
> > > bulk constructed by libgpiod.  Either way the user will have to free the
> > > bulk afterwards.  But there are only a few of those of those methods.
> > >
> >
> > Or we can even make these methods return struct gpiod_line_bulk *
> > which is cleaner than passing struct gpiod_line_bulk ** as argument
> > IMO. I'm fine with that.
> >
>
> Returning NULL for the error cases, as per gpiod_chip_find_line()?
>

Yes, definitely.

> > > And all the bulk accesor methods are currently inline - that would also
> > > have to change if you go opaque, so you will get a code size and
> > > performance hit as well.
> >
> > This may slightly increase the size of the library indeed but it will
> > decrease the size of the user programs. Anyway: we're talking about
> > miniscule changes. Performance hit isn't that much of a concern either
> > - function calls are fast and we're not adding new system calls that
> > could lead to more context switches.
> >
>
> Those inlines are just simple field accessors and mutators, and you'd hope
> that has less overhead than a function call.
>
> Opaque is certainly preferable - it reduces the API surface and you are
> also free to change the implementation later.
>
> Can we also add in range checking to functions that accept offsets while
> you are at it? e.g. gpiod_line_bulk_get_line() should return NULL if the
> offset is out of range. Or are we assuming that the user will always
> ensure offset < num_lines first?  If the latter can we at least document
> the bad behaviour if offset >= num_lines?
>

Yes, that's what I already have in my dev branch - functions that
return errors but I imagine, the users wouldn't be forced to check
these return values if they are sure the supplied arguments are
correct. If we ever use the warn_unused_result attribute in the
library - the bulk functions would not be annotated with it.

> > >
> > > Maybe keeping it exposed is the best - just heavily documented?
> > > At least start out that way, since it is closest to what is already there.
> > >
> >
> > I'm now actually leaning more towards making it opaque but I need to
> > find a way to make gpiod_line_bulk_foreach_line work with hidden bulk
> > struct.
> >
>
> Why not just drop it in favour of gpiod_line_bulk_foreach_line_off()?
>

The one with the line being supplied to the user automatically is more
elegant. If anything - I'd prefer to drop
gpiod_line_bulk_foreach_line_off(). Callbacks as suggested by Andy is
a good idea - something like what GLib does in a lot of helpers for
lists etc.

Bartosz