Re: [libgpiod v2][PATCH 2/6] API: rename gpiod_request_config_get_num_offsets to gpiod_request_config_get_num_lines to match line_request pattern

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Mar 15, 2022 at 12:39:56PM +0100, Bartosz Golaszewski wrote:
> On Tue, Mar 15, 2022 at 12:23 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Tue, Mar 15, 2022 at 11:52:21AM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Mar 11, 2022 at 8:40 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > >
> > > > Both gpiod_request_config and gpiod_line_request contain a number of
> > > > lines, but the former has a get_num_offsets accessor, while the latter
> > > > has get_num_lines.  Make them consistent by switching request_config to
> > > > get_num_lines.
> > > >
> > > > Signed-off-by: Kent Gibson <warthog618@xxxxxxxxx>
> > > > ---
> > >
> > > IMO this doesn't make much sense because we still have
> > > gpiod_request_config_set_offsets(). So now you have set_offsets but
> > > get_lines. At the time of preparing the request_config we're still
> > > talking about offsets of lines to request, while once the request has
> > > been made, we're talking about requested lines.
> > >
> >
> > Oh FFS we are always talking about lines.  Whether you have requested
> > them yet or not is irrelevant.  You WANT to request lines.
> > If we had globally unique line names we wouldn't give a rats about the
> > offset.
> >
> > And take another look - you have actually have get_offsets and
> > get_num_lines functions.
> >
> > Offsets is just one of the attributes of the lines, and this approach
> > still works if there is another fields of interest. e.g. values:
> >
> > int gpiod_line_request_set_values_subset(struct gpiod_line_request *request,
> >                                          size_t num_lines,
> >                                          const unsigned int *offsets,
> >                                          const int *values);
> >
> > which you then complain about in patch 4 as I'm writing this.... <sigh>.
> >
> > You could equally argue that one should be num_values.
> >
> > While we are still preparing the configuration, we are preparing the
> > config for LINES, not offsets.  Using num_lines is a reminder that you
> > need to provide the offset for each line - the two are inextricably
> > linked.  Using num_offsets could be taken to imply that
> > gpiod_request_config_set_offsets() can be called multiple times to add
> > offsets.
> >
> > > I would leave it as it is personally.
> > >
> >
> > I know, I know :-|.
> >
> > Cheers,
> > Kent.
> 
> I didn't know I would see the whole extend of Kent's wrath after that
> comment. :)
> 

We're still way way off the full extent.

Though "libgpiod v2 - the Wrath of Kent" does have a certain ring to it.

> Anyway let me try to defend myself before I wave the white flag and
> surrender as usual.
> 
> We're setting VALUES so it's only normal to speak about NUMBER of VALUES.
> 

But you are happy to call it num_offsets?  I'm confused.

> It's like when you have an array of of X that is an attribute of Y,
> that array still carries a number of X and not Y.
> 

I get that, but in this case the offset is identifier for line.
The mapping is 1-1.

> This is for patch 4 but this one has another problem. What if we
> extend this structure to - besides offsets - accept string identifiers
> for a request? Then we would want to use get_offsets and get_names (or
> get_ids) and the corresponding get_num_offsets and get_num_names
> accesors and in this case get_num_lines would become confusing.
> 

Good luck with that - no matter how you name things.
If you allow multiple identifiers then you have to deal with the
overlap case.  Just don't go there.
And what happens to gpiod_line_request_get_offsets where the size of
the buffer is determined by gpiod_line_request_get_num_lines()?

Much simpler to stick to a single type of identifier for the request.
Oh, and them the 1-1 mapping still holds, so you can still use num_lines.
No multi-dimensional thinking.

Cheers,
Kent.