Re: libgpiod: rust bindings and bindgen issue with C enums

[Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx>]

On Thu, Nov 24, 2022 at 2:32 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> I don't see this as a problem for generics.  Whether the enum is signed
> or unsigned doesn't need to affect the Error variant, much less the whole
> Error type.  The Error doesn't need to respresent the type of the source
> of the error, it needs to represent the type required to convey
> information to the user.
> Just accepting that the InvalidEnumValue variant expects i32, and casting
> from u32 if necessary, seems appropriate to me.  Unless there are some
> extreme values you are concerned about - but then you always switch it
> up to  i64 ;-).

Yeah, I am not sure what a generic `Error` buys us here.

If one really wants to preserve whether it is signed or not, that is
only two possibilities, not an open set of them. Moreover, I imagine
one wants to constraint them for users, rather than let users provide
the type `E`.

Thus one could have a sum type with 2 variants like
`InvalidEnumValue(..., Either<i32, u32>)` or something more explicit.

But that is assuming there is a need to preserve it. What is the
variant meant to be used for by users? e.g. if it is just for
reporting, it probably doesn't matter. Actually, does the user even
need the number? Could `InvalidArguments` be enough?

Looking at it a bit more, I am confused why the error is possible to
begin with. There is `Value::new()` which appears to be public and can
return `InvalidEnumValue`, but why should it be used by users? They
should create the `enum` directly, no? And for the other `new()`s that
also return `InvalidEnumValue`s, I see they are `pub(crate)`. That is
what I would expect, but still, why is the error a possibility to
begin with?

For instance, somewhere else the library does `Direction::new(...)`
with a value from the C side. The values from the C side must be
correct, i.e. it is a bug otherwise, right? Thus one can trust them,
or assert them, or if one wants to avoid panics for something that is
a bug, one could return a `InternalLibraryError` with no extra
information, because there is really not much users can do with the
error details apart from knowing there is a bug in either the Rust or
the C side.

I have taken a quick look at the C++ side for that same case, and from
a quick look, C++ appears to throw if the mappings are wrong, so it
sounds to me like you can similarly assert the validity in Rust and
remove the `InvalidEnumValue` variant altogether.

Cheers,
Miguel