Hi,
On Fri, Sep 29, 2023 at 03:41:27PM +0200, Victor Toso wrote:
> 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.
I was just double checking that the Union's value for each enum
branch needs to be a Struct. So I think it is fine to use boolean
for unhandled enum branches. I'll be doing that in the next
iteration.
docs/devel/qapi-code-gen.rst: 350
The BRANCH's value defines the branch's properties, in particular its
type. The type must a struct type. The form TYPE-REF_ is shorthand
for :code:`{ 'type': TYPE-REF }`.
Cheers,
Victor
signature.asc
Description: PGP signature
