| Message ID | 20190114130829.21790-2-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | typedefs: Remove scarcely used declarations | expand |
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: http://mid.mail-archive.com/32678400-47af-a842-0563-0d1a72712b4a@redhat.com See below. > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: Laszlo Ersek <lersek@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
On Mon, Jan 14, 2019 at 02:08:27PM +0100, 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. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: Laszlo Ersek <lersek@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(-) I am sorry I don't understand what this patchset is about. Supposed to be a cleanup but instead it pulls fw cfg into any user of unrelated headers. More lines of code slower builds what's to like? Shared typedefs is what typedefs.h is about. These are clearly shared so let's keep it simple. > diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h > index 38586ecbdf..be53de38f1 100644 > --- a/include/hw/acpi/vmgenid.h > +++ b/include/hw/acpi/vmgenid.h > @@ -3,6 +3,7 @@ > > #include "hw/acpi/bios-linker-loader.h" > #include "hw/qdev.h" > +#include "hw/nvram/fw_cfg.h" > #include "qemu/uuid.h" > > #define VMGENID_DEVICE "vmgenid" > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index 4cc57a7ef6..fef632dcf5 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -35,6 +35,7 @@ > #include "qemu/notify.h" > #include "hw/boards.h" > #include "hw/arm/arm.h" > +#include "hw/nvram/fw_cfg.h" > #include "sysemu/kvm.h" > #include "hw/intc/arm_gicv3_common.h" > > diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h > index c5c9b3c7f8..51d3a52c4b 100644 > --- a/include/hw/mem/nvdimm.h > +++ b/include/hw/mem/nvdimm.h > @@ -24,6 +24,7 @@ > #define QEMU_NVDIMM_H > > #include "hw/mem/pc-dimm.h" > +#include "hw/nvram/fw_cfg.h" > #include "hw/acpi/bios-linker-loader.h" > > #define NVDIMM_DEBUG 0 > 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) > > -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: > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index 30ad8b2d8d..293e47ef7c 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -28,10 +28,6 @@ typedef struct DriveInfo DriveInfo; > typedef struct Error Error; > typedef struct EventNotifier EventNotifier; > typedef struct FlatView FlatView; > -typedef struct FWCfgEntry FWCfgEntry; > -typedef struct FWCfgIoState FWCfgIoState; > -typedef struct FWCfgMemState FWCfgMemState; > -typedef struct FWCfgState FWCfgState; > typedef struct HVFX86EmulatorState HVFX86EmulatorState; > typedef struct I2CBus I2CBus; > typedef struct I2SCodec I2SCodec; > -- > 2.17.2 >
On 14/01/19 16:49, Michael S. Tsirkin wrote: > On Mon, Jan 14, 2019 at 02:08:27PM +0100, 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. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Reviewed-by: Laszlo Ersek <lersek@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(-) > > I am sorry I don't understand what this patchset is about. > Supposed to be a cleanup but instead it pulls > fw cfg into any user of unrelated headers. > > More lines of code slower builds what's to like? > > Shared typedefs is what typedefs.h is about. > These are clearly shared so let's keep it simple. I can see why this patch can be a bit controversial. Personally I think it's okay either way (which generally tends to favor the status quo). Paolo
On Mon, Jan 14, 2019 at 06:52:21PM +0100, Paolo Bonzini wrote: > On 14/01/19 16:49, Michael S. Tsirkin wrote: > > On Mon, Jan 14, 2019 at 02:08:27PM +0100, 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. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> Reviewed-by: Laszlo Ersek <lersek@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(-) > > > > I am sorry I don't understand what this patchset is about. > > Supposed to be a cleanup but instead it pulls > > fw cfg into any user of unrelated headers. > > > > More lines of code slower builds what's to like? > > > > Shared typedefs is what typedefs.h is about. > > These are clearly shared so let's keep it simple. > > I can see why this patch can be a bit controversial. Personally I think > it's okay either way (which generally tends to favor the status quo). > > Paolo True - not something to lose sleep over.
diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h index 38586ecbdf..be53de38f1 100644 --- a/include/hw/acpi/vmgenid.h +++ b/include/hw/acpi/vmgenid.h @@ -3,6 +3,7 @@ #include "hw/acpi/bios-linker-loader.h" #include "hw/qdev.h" +#include "hw/nvram/fw_cfg.h" #include "qemu/uuid.h" #define VMGENID_DEVICE "vmgenid" diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index 4cc57a7ef6..fef632dcf5 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -35,6 +35,7 @@ #include "qemu/notify.h" #include "hw/boards.h" #include "hw/arm/arm.h" +#include "hw/nvram/fw_cfg.h" #include "sysemu/kvm.h" #include "hw/intc/arm_gicv3_common.h" diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h index c5c9b3c7f8..51d3a52c4b 100644 --- a/include/hw/mem/nvdimm.h +++ b/include/hw/mem/nvdimm.h @@ -24,6 +24,7 @@ #define QEMU_NVDIMM_H #include "hw/mem/pc-dimm.h" +#include "hw/nvram/fw_cfg.h" #include "hw/acpi/bios-linker-loader.h" #define NVDIMM_DEBUG 0 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) -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: diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index 30ad8b2d8d..293e47ef7c 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -28,10 +28,6 @@ typedef struct DriveInfo DriveInfo; typedef struct Error Error; typedef struct EventNotifier EventNotifier; typedef struct FlatView FlatView; -typedef struct FWCfgEntry FWCfgEntry; -typedef struct FWCfgIoState FWCfgIoState; -typedef struct FWCfgMemState FWCfgMemState; -typedef struct FWCfgState FWCfgState; typedef struct HVFX86EmulatorState HVFX86EmulatorState; typedef struct I2CBus I2CBus; typedef struct I2SCodec I2SCodec;