Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Jan 16, 2023 at 10:37:14PM +0100, Bartosz Golaszewski wrote:
> On Mon, Jan 16, 2023 at 1:14 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Fri, Jan 13, 2023 at 10:51:58PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > >
> > > We have two functions in the C API that allow users to retrieve a list
> > > of offsets from objects: gpiod_line_request_get_offsets() and
> > > gpiod_line_config_get_offsets(). Even though they serve pretty much the
> > > same purpose, they have different signatures and one of them also
> > > requires the user to free the memory allocated within the libgpiod
> > > library with a non-libgpiod free() function.
> > >
> >
> > They differ because they operate in different circumstances.
> > Requests are immutable, wrt lines/offsets, while configs are not.
> > More on this below.
> >
> > > Unify them: make them take the array in which to store offsets and the
> > > size of this array. Make them return the number of offsets actually
> > > stored in the array and make them impossible to fail. Change their names
> > > to be more descriptive and in the case of line_config: add a new function
> > > that allows users to get the number of configured offsets.
> > >
> >
> > Not sure symmetry => beauty in this case.
> >
> > > Update the entire tree to use the new interfaces.
> > >
> > > For rust bindings: also unify the line config interface to return a map
> > > of line settings like C++ bindings do instead of having a function to
> > > get settings by offset. A map returned from a single call is easier to
> > > iterate over with a for loop than using an integer and calling the
> > > previous line_settings() method.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > <snip>
> > > --- a/include/gpiod.h
> > > +++ b/include/gpiod.h
> > > @@ -780,19 +780,29 @@ struct gpiod_line_settings *
> > >  gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
> > >                                   unsigned int offset);
> > >
> > > +/**
> > > + * @brief Get the number of configured line offsets.
> > > + * @param config Line config object.
> > > + * @return Number of offsets for which line settings have been added.
> > > + */
> > > +size_t
> > > +gpiod_line_config_get_num_configured_offsets(struct gpiod_line_config *config);
> > > +
> >               ^^^^^^         ^^^^^^^^^^
> > Not a fan of the stuttering.
> > What other kinds of lines are there in the config?
> 
> The user may not have an in-depth knowledge of the data model we use
> and just wants to use the library. I think this name is much more
> descriptive that way. It's not that long or repetetive, have you seen
> udev_monitor_filter_add_match_subsystem_devtype() or
> kmod_module_dependency_symbol_get_symbol()? :)
> 

If the data model is unclear to the user then you just made it less
clear as the implication from this naming is that there are OTHER types
of offsets/lines, but there is not.

I don't have a problem with your counter examples.  The first does not
stutter, and in the second the "symbol" appears to be the object, not an
adjective.  Length is not the issue - it is clarity. :|

But whatever - this is verging on bikeshedding.

Cheers,
Kent.