On Tue, Jun 01, 2021 at 09:57:08PM +0200, Bartosz Golaszewski wrote:

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

quoted

[peek]

quoted

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

Ahh, I forgot that you have the req_config and line_config split.
So you have no option but to maintain global flags.  That is used as
the initial value of the flags for a particular line when that one line is
first mutated, either by offset or as part of a subset.
And subsequent changes to the global flags need to be propagated to all
the lines in the config at that time.

Taking the most common flags value as the uAPI default is an optimization
that minimizes the number of lines requiring custom flag attributes in the
uAPI config. It might seem unintuitive to you, but it makes sense to me
that it should be the uAPI default, even if the user wasn't aware that it
should be.
OTOH there are only a few corner cases where it is of real benefit, so
just using the line_config global flags is fine.
FWIW, my Go library doesn't perform that optimization - it uses the
global flags :).  Not sure why I'm still suggesting it here when I
apparently decided it wasn't worth the effort there ¯\_(ツ)_/¯.

[snip]

quoted

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.

Fair enough - just another of those minor points that I would do
differently (I'd just stick with get and live with the fact that
returns ownership in some cases and not in others).

[snip]

quoted

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

Well that makes one of us ;).

Cheers,
Kent.