Re: [libgpiod][RFC v2] core: implement v2.0 API

On Sun, May 30, 2021 at 2:45 AM Kent Gibson [off-list ref] wrote:

[peek]

quoted

quoted

Eek this sounds vague. If that was part of documentation for this
function - as a user I would be confused. Is there any problem with
just implying output for any line for which a default value is set?

What part of "No need for the user to know or care" is too vague?
The user does not need to know how libgpiod translates to uAPI - that
is your problem not theirs, so don't go there.

But for your benefit...

There is no global direction setting - the lines are treated
independently until the translation to uAPI during the request or
reconfigure call, at which point the most common flags setting can be
used as the global default.

I said "vague" for want of a better word but what I mean is this:
taking the most common setting as the global default does not sound
intuitive and -  when coding - it would be hard to predict the outcome
from just looking at the code. I was actually assuming that we'd have
lines begin with a certain sane, default config (INPUT, active-high,
no bias, no drive, no edge etc) and every line's config would have to
be changed explicitly - either via the global mutators or the ones
that work with a subset of lines (where the subset could actually
include all the lines - we don't know it before we do the actual
request because the list of lines to request is passed in a different
struct).

[snip]

quoted

quoted

I forgot to ask about where gpiod_line_info_get_name() and others that
return char * fit wrt that pattern.
So a string isn't a complex object?
Maybe they should be _peek_ as well?
Either way, it would be nice for their commentary to describe the lifetime
of the returned pointer.

With strings the common sense is to assume that returning char * means
the string is dynamically allocated and must be freed by the caller,
returning const char * means the string is stored in the container. I
can't really recall seeing any other pattern in any sane C library. In
any case - I will add a comment to every function that returns an
object that needs lifetime management from the caller in v3.

Sure, but don't assume common sense - document the behaviour - even if
the returned object doesn't require lifetime management by the caller.
They might assume they do - and free it for you.

And the point was that you still have some gets that return objects while
others return a reference - even after renaming some to peek, so the
pattern doesn't follow for the whole libgpiod API. Granted that is
nitpicking, but I would prefer internal self-consistency over following
what other libraries do.

I don't want to use "peek" for simple types if that's what you're
hinting at because that would be very confusing to anyone experienced
with other C linux libraries. I would like to go with "get" for
functions returning pointers to opaque structs that need management
and peek for those that return pointers to structs stored in other
objects. I don't think we need to have additional "copy" functions
operating on the copied objects.

[snip]

quoted

quoted

Having error-less mutators here would mean something like a hundred
functions just for setting the line config. I think that even with
mutators taking enums we already have enough symbols. Let's keep them.

We don't want separate mutators for each enum value as that would
explode the symbol space.  Agreed.

To be clear, in my "trade-off" paragraph above I was referring to your
existing parameterised error-less mutators, not the parameter-less
error-less mutators that I had been assuming.
Different paragraph and different things.

So, to conclude, I would lean towards keeping your existing mutators that
don't return error codes, rather than adding error codes.
And only performing the validation when the config is translated to uAPI,
not providing some functions to perform interim validation.
i.e. wrt error handling I'm fine with the mutators as they are.

Ok got it.

I think we're getting close to an agreement. :)

Bart