Re: [Qemu-ppc] [Qemu-devel] [PATCH for-3.2 v3 02/14] qom: make interface types abstract

[Igor Mammedov]

On Tue, 20 Nov 2018 19:54:23 +0100
Laszlo Ersek <address@hidden> wrote:

> On 11/20/18 17:33, Igor Mammedov wrote:
> > On Wed,  7 Nov 2018 16:36:40 +0400
> > Marc-André Lureau <address@hidden> wrote:
> >   
> >> Interfaces don't have instance, let's make the interface type really
> >> abstract to avoid confusion.
> >>
> >> Signed-off-by: Marc-André Lureau <address@hidden>
> >> ---
> >>  include/hw/acpi/acpi_dev_interface.h | 6 +-----
> >>  include/hw/arm/linux-boot-if.h       | 5 +----
> >>  include/hw/fw-path-provider.h        | 4 +---
> >>  include/hw/hotplug.h                 | 6 +-----
> >>  include/hw/intc/intc.h               | 4 +---
> >>  include/hw/ipmi/ipmi.h               | 4 +---
> >>  include/hw/isa/isa.h                 | 4 ----
> >>  include/hw/mem/memory-device.h       | 4 +---
> >>  include/hw/nmi.h                     | 4 +---
> >>  include/hw/stream.h                  | 4 +---
> >>  include/hw/timer/m48t59.h            | 4 +---
> >>  include/qom/object_interfaces.h      | 6 +-----
> >>  include/sysemu/tpm.h                 | 4 +---
> >>  target/arm/idau.h                    | 4 +---
> >>  tests/check-qom-interface.c          | 4 +---
> >>  15 files changed, 14 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/include/hw/acpi/acpi_dev_interface.h 
> >> b/include/hw/acpi/acpi_dev_interface.h
> >> index dabf4c4fc9..43ff119179 100644
> >> --- a/include/hw/acpi/acpi_dev_interface.h
> >> +++ b/include/hw/acpi/acpi_dev_interface.h
> >> @@ -25,11 +25,7 @@ typedef enum {
> >>       INTERFACE_CHECK(AcpiDeviceIf, (obj), \
> >>                       TYPE_ACPI_DEVICE_IF)
> >>  
> >> -
> >> -typedef struct AcpiDeviceIf {
> >> -    /* <private> */
> >> -    Object Parent;
> >> -} AcpiDeviceIf;
> >> +typedef struct AcpiDeviceIf AcpiDeviceIf;
> >>  
> >>  void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event);
> >>  
> >> diff --git a/include/hw/arm/linux-boot-if.h 
> >> b/include/hw/arm/linux-boot-if.h
> >> index aba4479a14..7bbdfd1cc6 100644
> >> --- a/include/hw/arm/linux-boot-if.h
> >> +++ b/include/hw/arm/linux-boot-if.h
> >> @@ -16,10 +16,7 @@
> >>  #define ARM_LINUX_BOOT_IF(obj) \
> >>      INTERFACE_CHECK(ARMLinuxBootIf, (obj), TYPE_ARM_LINUX_BOOT_IF)
> >>  
> >> -typedef struct ARMLinuxBootIf {
> >> -    /*< private >*/
> >> -    Object parent_obj;
> >> -} ARMLinuxBootIf;
> >> +typedef struct ARMLinuxBootIf ARMLinuxBootIf;  
> > I like how it makes interface truly opaque and removes the need for
> > structure declaration but:
> > 
> >  1: I'm not sure if it's acceptable thing to do from language point of view 
> >  
> 
> Yeah, it's fine. If you have just
> 
> struct ARMLinuxBootIf;
> 
> (and, optionally, a typedef to it,) then this type is called an
> "incomplete type" (for translation units that don't see the actual type
> definition). You can't apply the "sizeof" operator to it, you can't put
> it in other structs and arrays etc. I'm too lazy to look up the exact
> details in the C standard now. :) But, importantly,
> "pointer-to-ARMLinuxBootIf" is a complete type, and you can do all the
> usual things with that. (Define variables of that pointer type, embed
> them in other structures, use it as an array element type, pass them to
> functions, and so on.)

Thanks Laszlo, that's the answer I was looking for.

> Thanks
> Laszlo
> 
> >  2: For a reader not aware of a trick, it's sort of confusing to have 
> > forward declaration but without structure itself. So if #1 is acceptable we 
> > probably should document interface trick in object.h
> > 
> > [...]
> >   
> 
>