Hi Phil, On 01/14/19 14:08, Philippe Mathieu-Daudé wrote: > There are only three include files requiring these typedefs, let them > include "hw/nvram/fw_cfg.h" directly to simplify "qemu/typedefs.h". > > To clean "qemu/typedefs.h", move the declarations to "hw/nvram/fw_cfg.h". > Reorder two function declarations to avoid forward typedef declarations.
No, this is not what I meant. I didn't suggest that we should remove forward declarations. I suggested that we should place the forward declarations *ahead of* the first references to them: 32678400-47af-a842-0563-0d1a72712b4a@redhat.com">http://mid.mail-archive.com/32678400-47af-a842-0563-0d1a72712b4a@redhat.com See below. > Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > --- > include/hw/acpi/vmgenid.h | 1 + > include/hw/arm/virt.h | 1 + > include/hw/mem/nvdimm.h | 1 + > include/hw/nvram/fw_cfg.h | 22 ++++++++++++---------- > include/qemu/typedefs.h | 4 ---- > 5 files changed, 15 insertions(+), 14 deletions(-) [...] > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index f5a6895a74..cc744d5268 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -14,15 +14,12 @@ > #define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) > #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) On these lines, you have references to FWCfgState, FWCfgIoState, FWCfgMemState. Pre-patch, that is OK, because you get the types via the #includes, *and* the #include directives come first. (Note: this is not about compilation safety; it is about how a human reads this file.) Post-patch, however: > > -typedef struct fw_cfg_file FWCfgFile; > - > #define FW_CFG_ORDER_OVERRIDE_VGA 70 > #define FW_CFG_ORDER_OVERRIDE_NIC 80 > #define FW_CFG_ORDER_OVERRIDE_USER 100 > #define FW_CFG_ORDER_OVERRIDE_DEVICE 110 > > -void fw_cfg_set_order_override(FWCfgState *fw_cfg, int order); > -void fw_cfg_reset_order_override(FWCfgState *fw_cfg); > +typedef struct fw_cfg_file FWCfgFile; > > typedef struct FWCfgFiles { > uint32_t count; > @@ -34,7 +31,9 @@ typedef struct fw_cfg_dma_access FWCfgDmaAccess; > typedef void (*FWCfgCallback)(void *opaque); > typedef void (*FWCfgWriteCallback)(void *opaque, off_t start, size_t len); > > -struct FWCfgState { > +typedef struct FWCfgEntry FWCfgEntry; > + > +typedef struct FWCfgState { > /*< private >*/ > SysBusDevice parent_obj; > /*< public >*/ > @@ -53,17 +52,17 @@ struct FWCfgState { > dma_addr_t dma_addr; > AddressSpace *dma_as; > MemoryRegion dma_iomem; > -}; > +} FWCfgState; > > -struct FWCfgIoState { > +typedef struct FWCfgIoState { > /*< private >*/ > FWCfgState parent_obj; > /*< public >*/ > > MemoryRegion comb_iomem; > -}; > +} FWCfgIoState; > > -struct FWCfgMemState { > +typedef struct FWCfgMemState { > /*< private >*/ > FWCfgState parent_obj; > /*< public >*/ > @@ -71,7 +70,10 @@ struct FWCfgMemState { > MemoryRegion ctl_iomem, data_iomem; > uint32_t data_width; > MemoryRegionOps wide_data_ops; > -}; > +} FWCfgMemState; > + > +void fw_cfg_set_order_override(FWCfgState *fw_cfg, int order); > +void fw_cfg_reset_order_override(FWCfgState *fw_cfg); > > /** > * fw_cfg_add_bytes: the type names are introduced only later. Thus, when a human reads the file, the type references in the OBJECT_CHECK() macros appear before the same type names are introduced in any way. What I meant, for v1, was simply: > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index 244ed78afafb..063375e07b2d 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -5,20 +5,20 @@ > #include "standard-headers/linux/qemu_fw_cfg.h" > #include "hw/sysbus.h" > #include "sysemu/dma.h" > > +typedef struct FWCfgState FWCfgState; > +typedef struct FWCfgIoState FWCfgIoState; > +typedef struct FWCfgMemState FWCfgMemState; > + > #define TYPE_FW_CFG "fw_cfg" > #define TYPE_FW_CFG_IO "fw_cfg_io" > #define TYPE_FW_CFG_MEM "fw_cfg_mem" > > #define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG) > #define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) > #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) > > -typedef struct FWCfgState FWCfgState; > -typedef struct FWCfgIoState FWCfgIoState; > -typedef struct FWCfgMemState FWCfgMemState; > - > #define FW_CFG_ORDER_OVERRIDE_VGA 70 > #define FW_CFG_ORDER_OVERRIDE_NIC 80 > #define FW_CFG_ORDER_OVERRIDE_USER 100 > #define FW_CFG_ORDER_OVERRIDE_DEVICE 110 Now, if you argued that this was not idiomatic for QEMU, or it was undesirable for some other reason, that could be a valid observation, and then we should discuss it further. My point here is that I didn't intend my v1 suggestion as, or my R-b for, the code that's visible in v2. Thanks, Laszlo
