| Message ID | 1466698330-6021-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 23 June 2016 at 17:12, Markus Armbruster <armbru@redhat.com> wrote: > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > include/hw/irq.h | 2 -- > include/qemu/typedefs.h | 1 + > tests/Makefile.include | 2 -- > 3 files changed, 1 insertion(+), 4 deletions(-) > > diff --git a/include/hw/irq.h b/include/hw/irq.h > index 4c4c2ea..6a89571 100644 > --- a/include/hw/irq.h > +++ b/include/hw/irq.h > @@ -5,8 +5,6 @@ > > #define TYPE_IRQ "irq" > > -typedef struct IRQState *qemu_irq; > - > typedef void (*qemu_irq_handler)(void *opaque, int n, int level); > > void qemu_set_irq(qemu_irq irq, int level); > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index f9745c4..f7c27a9 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -31,6 +31,7 @@ typedef struct FWCfgState FWCfgState; > typedef struct HCIInfo HCIInfo; > typedef struct I2CBus I2CBus; > typedef struct I2SCodec I2SCodec; > +typedef struct IRQState *qemu_irq; > typedef struct ISABus ISABus; > typedef struct ISADevice ISADevice; > typedef struct IsaDma IsaDma; Everything else in typedefs.h is a "typedef struct Thing Thing", but qemu_irq is different... thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 23 June 2016 at 17:12, Markus Armbruster <armbru@redhat.com> wrote: >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> include/hw/irq.h | 2 -- >> include/qemu/typedefs.h | 1 + >> tests/Makefile.include | 2 -- >> 3 files changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/include/hw/irq.h b/include/hw/irq.h >> index 4c4c2ea..6a89571 100644 >> --- a/include/hw/irq.h >> +++ b/include/hw/irq.h >> @@ -5,8 +5,6 @@ >> >> #define TYPE_IRQ "irq" >> >> -typedef struct IRQState *qemu_irq; >> - >> typedef void (*qemu_irq_handler)(void *opaque, int n, int level); >> >> void qemu_set_irq(qemu_irq irq, int level); >> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >> index f9745c4..f7c27a9 100644 >> --- a/include/qemu/typedefs.h >> +++ b/include/qemu/typedefs.h >> @@ -31,6 +31,7 @@ typedef struct FWCfgState FWCfgState; >> typedef struct HCIInfo HCIInfo; >> typedef struct I2CBus I2CBus; >> typedef struct I2SCodec I2SCodec; >> +typedef struct IRQState *qemu_irq; >> typedef struct ISABus ISABus; >> typedef struct ISADevice ISADevice; >> typedef struct IsaDma IsaDma; > > Everything else in typedefs.h is a "typedef struct Thing Thing", > but qemu_irq is different... We want to keep our readers on their toes!
On 24 June 2016 at 09:12, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 23 June 2016 at 17:12, Markus Armbruster <armbru@redhat.com> wrote: >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> include/hw/irq.h | 2 -- >>> include/qemu/typedefs.h | 1 + >>> tests/Makefile.include | 2 -- >>> 3 files changed, 1 insertion(+), 4 deletions(-) >>> >>> diff --git a/include/hw/irq.h b/include/hw/irq.h >>> index 4c4c2ea..6a89571 100644 >>> --- a/include/hw/irq.h >>> +++ b/include/hw/irq.h >>> @@ -5,8 +5,6 @@ >>> >>> #define TYPE_IRQ "irq" >>> >>> -typedef struct IRQState *qemu_irq; >>> - >>> typedef void (*qemu_irq_handler)(void *opaque, int n, int level); >>> >>> void qemu_set_irq(qemu_irq irq, int level); >>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >>> index f9745c4..f7c27a9 100644 >>> --- a/include/qemu/typedefs.h >>> +++ b/include/qemu/typedefs.h >>> @@ -31,6 +31,7 @@ typedef struct FWCfgState FWCfgState; >>> typedef struct HCIInfo HCIInfo; >>> typedef struct I2CBus I2CBus; >>> typedef struct I2SCodec I2SCodec; >>> +typedef struct IRQState *qemu_irq; >>> typedef struct ISABus ISABus; >>> typedef struct ISADevice ISADevice; >>> typedef struct IsaDma IsaDma; >> >> Everything else in typedefs.h is a "typedef struct Thing Thing", >> but qemu_irq is different... > > We want to keep our readers on their toes! It would mean you now have to decide whether the file is orderd by the types being defined or by the underlying implementation type (previously both orders were the same)... thanks -- PMM
On 24/06/2016 10:15, Peter Maydell wrote: >>>> >>> @@ -31,6 +31,7 @@ typedef struct FWCfgState FWCfgState; >>>> >>> typedef struct HCIInfo HCIInfo; >>>> >>> typedef struct I2CBus I2CBus; >>>> >>> typedef struct I2SCodec I2SCodec; >>>> >>> +typedef struct IRQState *qemu_irq; >>>> >>> typedef struct ISABus ISABus; >>>> >>> typedef struct ISADevice ISADevice; >>>> >>> typedef struct IsaDma IsaDma; >>> >> >>> >> Everything else in typedefs.h is a "typedef struct Thing Thing", >>> >> but qemu_irq is different... >> > >> > We want to keep our readers on their toes! > It would mean you now have to decide whether the file is orderd > by the types being defined or by the underlying implementation > type (previously both orders were the same)... Indeed, and renaming the struct is trivial because it's used in a handful of places only. Paolo
On 24 June 2016 at 09:17, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 24/06/2016 10:15, Peter Maydell wrote: >>>>> >>> @@ -31,6 +31,7 @@ typedef struct FWCfgState FWCfgState; >>>>> >>> typedef struct HCIInfo HCIInfo; >>>>> >>> typedef struct I2CBus I2CBus; >>>>> >>> typedef struct I2SCodec I2SCodec; >>>>> >>> +typedef struct IRQState *qemu_irq; >>>>> >>> typedef struct ISABus ISABus; >>>>> >>> typedef struct ISADevice ISADevice; >>>>> >>> typedef struct IsaDma IsaDma; >>>> >> >>>> >> Everything else in typedefs.h is a "typedef struct Thing Thing", >>>> >> but qemu_irq is different... >>> > >>> > We want to keep our readers on their toes! >> It would mean you now have to decide whether the file is orderd >> by the types being defined or by the underlying implementation >> type (previously both orders were the same)... > > Indeed, and renaming the struct is trivial because it's used in a > handful of places only. It would still be different by being a pointer-to-Foo, not a Foo. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 24 June 2016 at 09:17, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 24/06/2016 10:15, Peter Maydell wrote: >>>>>> >>> @@ -31,6 +31,7 @@ typedef struct FWCfgState FWCfgState; >>>>>> >>> typedef struct HCIInfo HCIInfo; >>>>>> >>> typedef struct I2CBus I2CBus; >>>>>> >>> typedef struct I2SCodec I2SCodec; >>>>>> >>> +typedef struct IRQState *qemu_irq; >>>>>> >>> typedef struct ISABus ISABus; >>>>>> >>> typedef struct ISADevice ISADevice; >>>>>> >>> typedef struct IsaDma IsaDma; >>>>> >> >>>>> >> Everything else in typedefs.h is a "typedef struct Thing Thing", >>>>> >> but qemu_irq is different... >>>> > >>>> > We want to keep our readers on their toes! >>> It would mean you now have to decide whether the file is orderd >>> by the types being defined or by the underlying implementation >>> type (previously both orders were the same)... >> >> Indeed, and renaming the struct is trivial because it's used in a >> handful of places only. > > It would still be different by being a pointer-to-Foo, not a Foo. Hiding pointer-ness behind a typedef is a bad idea more often than not. What do you want me to do, if anything?
On 24 June 2016 at 13:32, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> It would still be different by being a pointer-to-Foo, not a Foo. > > Hiding pointer-ness behind a typedef is a bad idea more often than not. > > What do you want me to do, if anything? I think my underlying issue is that the purpose of typedefs.h (as I see it) is to define some typedefs for handing around pointers to opaque objects, but we don't pass around pointers to qemu_irqs, we pass around actual qemu_irqs. So I don't really feel like it belongs in this header. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 24 June 2016 at 13:32, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >>> It would still be different by being a pointer-to-Foo, not a Foo. >> >> Hiding pointer-ness behind a typedef is a bad idea more often than not. >> >> What do you want me to do, if anything? > > I think my underlying issue is that the purpose of typedefs.h > (as I see it) is to define some typedefs for handing around > pointers to opaque objects, but we don't pass around pointers > to qemu_irqs, we pass around actual qemu_irqs. So I don't > really feel like it belongs in this header. My view of typedefs.h is more pragmatic: it's for breaking inclusion cycles at an arc where we get a complete type from FOO.h, but only need an incomplete one: declare the incomplete type in typedefs.h, complete it in FOO.h, drop inclusions of FOO.h that don't need the complete type. Can also be done by creating a FOO-abstract.h that declares the incomplete type and whatever else makes sense. Overkill when "whatever else" is empty, which it often is. Dropping inclusions that are just for the incomplete can also speed up compilation. If you really don't want qemu_irq in typedefs.h, where should it go?
On 24 June 2016 at 14:43, Markus Armbruster <armbru@redhat.com> wrote:
> If you really don't want qemu_irq in typedefs.h, where should it go?
Last time I had a typedef like that I wrote a header for it:
include/qemu/fprintf-fn.h...
thanks
-- PMM
diff --git a/include/hw/irq.h b/include/hw/irq.h index 4c4c2ea..6a89571 100644 --- a/include/hw/irq.h +++ b/include/hw/irq.h @@ -5,8 +5,6 @@ #define TYPE_IRQ "irq" -typedef struct IRQState *qemu_irq; - typedef void (*qemu_irq_handler)(void *opaque, int n, int level); void qemu_set_irq(qemu_irq irq, int level); diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index f9745c4..f7c27a9 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -31,6 +31,7 @@ typedef struct FWCfgState FWCfgState; typedef struct HCIInfo HCIInfo; typedef struct I2CBus I2CBus; typedef struct I2SCodec I2SCodec; +typedef struct IRQState *qemu_irq; typedef struct ISABus ISABus; typedef struct ISADevice ISADevice; typedef struct IsaDma IsaDma; diff --git a/tests/Makefile.include b/tests/Makefile.include index e20f437..cd2082a 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -543,11 +543,9 @@ blacklisted-headers := \ include/hw/input/hid.h \ include/hw/intc/allwinner-a10-pic.h \ include/hw/isa/i8257.h \ - include/hw/isa/vt82c686.h \ include/hw/kvm/clock.h \ include/hw/mips/bios.h \ include/hw/mips/cpudevs.h \ - include/hw/mips/mips.h \ include/hw/misc/mips_cmgcr.h \ include/hw/misc/mips_cpc.h \ include/hw/misc/mips_itu.h \
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/hw/irq.h | 2 -- include/qemu/typedefs.h | 1 + tests/Makefile.include | 2 -- 3 files changed, 1 insertion(+), 4 deletions(-)