Hi, On Thu, Sep 28, 2023 at 03:21:59PM +0100, Daniel P. Berrangé wrote: > On Wed, Sep 27, 2023 at 01:25:40PM +0200, Victor Toso wrote: > > This patch handles QAPI union types and generates the equivalent data > > structures and methods in Go to handle it. > > > > The QAPI union type has two types of fields: The @base and the > > @Variants members. The @base fields can be considered common members > > for the union while only one field maximum is set for the @Variants. > > > > In the QAPI specification, it defines a @discriminator field, which is > > an Enum type. The purpose of the @discriminator is to identify which > > @variant type is being used. > > > > Not that @discriminator's enum might have more values than the union's > > data struct. This is fine. The union does not need to handle all cases > > of the enum, but it should accept them without error. For this > > specific case, we keep the @discriminator field in every union type. > > I still tend think the @discriminator field should not be > present in the union structs. It feels like we're just trying > to directly copy the C code in Go and so smells wrong from a > Go POV. > > For most of the unions the @discriminator field will be entirely > redundant, becasue the commonm case is that a @variant field > exists for every possible @discriminator value.
You are correct.
> To take one example
>
> type SocketAddress struct {
> Type SocketAddressType `json:"type"`
>
> // Variants fields
> Inet *InetSocketAddress `json:"-"`
> Unix *UnixSocketAddress `json:"-"`
> Vsock *VsockSocketAddress `json:"-"`
> Fd *String `json:"-"`
> }
>
> If one was just writing Go code without the pre-existing knowledge
> of the QAPI C code, 'Type' is not something a Go programmer would
> be inclined add IMHO.
You don't need previous knowledge in the QAPI C code to see that
having optional field members and a discriminator field feels
very very suspicious. I wasn't too happy to add it.
> And yet you are right that we need a way to represent a
> @discriminator value that has no corresponding @variant, since
> QAPI allows for that scenario.
Thank Markus for that, really nice catch :)
> To deal with that I would suggest we just use an empty
> interface type. eg
>
> type SocketAddress struct {
> Type SocketAddressType `json:"type"`
>
> // Variants fields
> Inet *InetSocketAddress `json:"-"`
> Unix *UnixSocketAddress `json:"-"`
> Vsock *VsockSocketAddress `json:"-"`
> Fd *String `json:"-"`
> Fish *interface{} `json:"-"`
> Food *interface() `json:"-"`
> }
>
> the pointer value for 'Fish' and 'Food' fields here merely needs to
> be non-NULL, it doesn't matter what the actual thing assigned is.
I like this idea. What happens if Fish becomes a handled in the
future?
Before:
type SocketAddress struct {
// Variants fields
Inet *InetSocketAddress `json:"-"`
Unix *UnixSocketAddress `json:"-"`
Vsock *VsockSocketAddress `json:"-"`
Fd *String `json:"-"`
// Unhandled enum branches
Fish *interface{} `json:"-"`
Food *interface{} `json:"-"`
}
to
type SocketAddress struct {
// Variants fields
Inet *InetSocketAddress `json:"-"`
Unix *UnixSocketAddress `json:"-"`
Vsock *VsockSocketAddress `json:"-"`
Fd *String `json:"-"`
Fish *FishSocketAddress `json:"-"`
// Unhandled enum branches
Food *interface{} `json:"-"`
}
An application that hat s.Fish = &something, will now error on
compile due something type not being FishSocketAddress. I think
this is acceptable. Very corner case scenario and the user
probably want to use the right struct now.
If you agree with above, I'd instead like to try a boolean
instead of *interface{}. s.Fish = true seems better and false is
simply ignored.
Cheers,
Victor
signature.asc
Description: PGP signature
