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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Mar 15, 2022 at 12:59 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> 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.
>

Love it, let's make it official. :)

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

Wat? No, the only num_offsets are present in get/set_offsets in request_config.

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

The string identifiers would be translated to offsets at some point.
Here it would make sense to retrieve the number of lines ACTUALLY
requested and get their OFFSETS of which there are 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.
>

I don't see a problem with current naming. You set offsets ->
num_offsets, values -> num_values. Also: unlike function names this is
not even part of ABI. We can even name it `n`, `nelem`, `num_elem`
everywhere for all I care.

Bart