Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go

[Andrea Bolognani]

On Tue, Jul 05, 2022 at 05:47:25PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote:
> > On Fri, Jun 17, 2022 at 02:19:29PM +0200, Victor Toso wrote:
> > > func (s *AcpiDeviceOstEvent) GetTimestamp() Timestamp {
> > >     return s.EventTimestamp
> > > }
> >
> > Does this even need a getter? The struct member is public, and Go
> > code seems to favor direct member access.
>
> It satisfies the 'Event' interface, so you can fetch timestamp
> without needing to know the specific event struct you're dealing
> with.

Yeah but we're generating structs for all possible events ourselves
and we don't really expect external implementations of this
interface so I don't see what having this getter buys us over the
guarantee, that we have by construction, that all events will have a
public member with a specific name that contains the timestamp.

> I don't think this kind of detail really needs to be exposed to
> clients. Also parsing the same json doc twice just feels wrong.
>
> I think using the dummy union structs is the right approach, but
> I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to
> make it clear this is different from a normal 'UnmarshalJSON'
> method.

Fair point on avoiding parsing the JSON twice.

I still don't like the fact that we have to force clients to use a
non-standard API, or that the API for events has to be different from
that for unions. But Go won't let you add method implementations to
an interface, so we're kinda stuck.

The only alternative I can think of would be to define Event as

  type Event struct {
      Shutdown *ShutdownEvent
      Reset    *ResetEvent
      // and so on
  }

which wouldn't be too bad from client code, as you'd only have to
change from

  e, err := EventFromJSON(in)
  switch v := e.(type) {
      case ShutdownEvent:
         // do something with v
      case ResetEvent:
         // do something with v
      // and so on
  }

to

  err := json.UnmarshalJSON(in, &e)
  if e.Shutdown != nil {
      // do something with e.Shutdown
  } else if e.Reset != nil {
      // do something with e.Reset
  } // and so on

but that would mean each time we have to parse an event we'd end up
allocating room for ~50 pointers and only actually using one, which
is a massive waste.

-- 
Andrea Bolognani / Red Hat / Virtualization