| Message ID | 20190812052359.30071-12-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Tame a few "touch this, recompile the world" headers | expand |
Markus Armbruster <armbru@redhat.com> writes: > While there, drop the obsolete file comment. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/qemu/typedefs.h | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index fcdaae58c4..29346648d4 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -1,10 +1,10 @@ > #ifndef QEMU_TYPEDEFS_H > #define QEMU_TYPEDEFS_H > > -/* A load of opaque types so that device init declarations don't have to > - pull in all the real definitions. */ > - > -/* Please keep this list in case-insensitive alphabetical order */ > +/* > + * Incomplete struct types Maybe expand this a little... "Incomplete struct types for modules that don't need the complete definitions but still pass around typed variables."? Otherwise: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > + * Please keep this list in case-insensitive alphabetical order. > + */ > typedef struct AdapterInfo AdapterInfo; > typedef struct AddressSpace AddressSpace; > typedef struct AioContext AioContext; > @@ -101,6 +101,10 @@ typedef struct SHPCDevice SHPCDevice; > typedef struct SSIBus SSIBus; > typedef struct VirtIODevice VirtIODevice; > typedef struct Visitor Visitor; > + > +/* > + * Function types > + */ > typedef void SaveStateHandler(QEMUFile *f, void *opaque); > typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id); -- Alex Bennée
Alex Bennée <alex.bennee@linaro.org> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> While there, drop the obsolete file comment. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> include/qemu/typedefs.h | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >> index fcdaae58c4..29346648d4 100644 >> --- a/include/qemu/typedefs.h >> +++ b/include/qemu/typedefs.h >> @@ -1,10 +1,10 @@ >> #ifndef QEMU_TYPEDEFS_H >> #define QEMU_TYPEDEFS_H >> >> -/* A load of opaque types so that device init declarations don't have to >> - pull in all the real definitions. */ >> - >> -/* Please keep this list in case-insensitive alphabetical order */ >> +/* >> + * Incomplete struct types > > Maybe expand this a little... > > "Incomplete struct types for modules that don't need the complete > definitions but still pass around typed variables."? If we explain proper use of qemu/typedefs.h in HACKING, as discussed in review of v1[*], we could point there. > Otherwise: > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Thanks! >> + * Please keep this list in case-insensitive alphabetical order. >> + */ >> typedef struct AdapterInfo AdapterInfo; >> typedef struct AddressSpace AddressSpace; >> typedef struct AioContext AioContext; >> @@ -101,6 +101,10 @@ typedef struct SHPCDevice SHPCDevice; >> typedef struct SSIBus SSIBus; >> typedef struct VirtIODevice VirtIODevice; >> typedef struct Visitor Visitor; >> + >> +/* >> + * Function types >> + */ >> typedef void SaveStateHandler(QEMUFile *f, void *opaque); >> typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id); > > > -- > Alex Bennée
Markus Armbruster <armbru@redhat.com> writes: > Alex Bennée <alex.bennee@linaro.org> writes: > >> Markus Armbruster <armbru@redhat.com> writes: >> >>> While there, drop the obsolete file comment. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> include/qemu/typedefs.h | 12 ++++++++---- >>> 1 file changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >>> index fcdaae58c4..29346648d4 100644 >>> --- a/include/qemu/typedefs.h >>> +++ b/include/qemu/typedefs.h >>> @@ -1,10 +1,10 @@ >>> #ifndef QEMU_TYPEDEFS_H >>> #define QEMU_TYPEDEFS_H >>> >>> -/* A load of opaque types so that device init declarations don't have to >>> - pull in all the real definitions. */ >>> - >>> -/* Please keep this list in case-insensitive alphabetical order */ >>> +/* >>> + * Incomplete struct types >> >> Maybe expand this a little... >> >> "Incomplete struct types for modules that don't need the complete >> definitions but still pass around typed variables."? > > If we explain proper use of qemu/typedefs.h in HACKING, as discussed in > review of v1[*], we could point there. Perhaps rewriting the obsolete file comment would be better. Something like this: /* * This header is for selectively avoiding #include just to get a * typedef name. * * Declaring a typedef name in its "obvious" place can result in * inclusion cycles, in particular for complete struct and union * types that need more types for their members. It can also result * in headers pulling in many more headers, slowing down builds. * * You can break such cycles and unwanted dependencies by declaring * the typedef name here. * * For struct types used in only a few headers, judicious use of the * struct tag instead of the typedef name is commonly preferable. */ /* * Incomplete struct types * Please keep this list in case-insensitive alphabetical order. */ typedef struct AdapterInfo AdapterInfo; [...] /* * Pointer types * Such typedefs should be limited to cases where the typedef's users * are oblivious of its "pointer-ness". * Please keep this list in case-insensitive alphabetical order. */ typedef struct IRQState *qemu_irq; /* * Function types */ typedef void SaveStateHandler(QEMUFile *f, void *opaque); typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id); typedef void (*qemu_irq_handler)(void *opaque, int n, int level); What do you think? [...]
Markus Armbruster <armbru@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Alex Bennée <alex.bennee@linaro.org> writes: >> >>> Markus Armbruster <armbru@redhat.com> writes: >>> >>>> While there, drop the obsolete file comment. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> include/qemu/typedefs.h | 12 ++++++++---- >>>> 1 file changed, 8 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >>>> index fcdaae58c4..29346648d4 100644 >>>> --- a/include/qemu/typedefs.h >>>> +++ b/include/qemu/typedefs.h >>>> @@ -1,10 +1,10 @@ >>>> #ifndef QEMU_TYPEDEFS_H >>>> #define QEMU_TYPEDEFS_H >>>> >>>> -/* A load of opaque types so that device init declarations don't have to >>>> - pull in all the real definitions. */ >>>> - >>>> -/* Please keep this list in case-insensitive alphabetical order */ >>>> +/* >>>> + * Incomplete struct types >>> >>> Maybe expand this a little... >>> >>> "Incomplete struct types for modules that don't need the complete >>> definitions but still pass around typed variables."? >> >> If we explain proper use of qemu/typedefs.h in HACKING, as discussed in >> review of v1[*], we could point there. > > Perhaps rewriting the obsolete file comment would be better. Something > like this: > > /* > * This header is for selectively avoiding #include just to get a > * typedef name. > * > * Declaring a typedef name in its "obvious" place can result in > * inclusion cycles, in particular for complete struct and union > * types that need more types for their members. It can also result > * in headers pulling in many more headers, slowing down builds. > * > * You can break such cycles and unwanted dependencies by declaring > * the typedef name here. > * > * For struct types used in only a few headers, judicious use of the > * struct tag instead of the typedef name is commonly preferable. > */ > > /* > * Incomplete struct types > * Please keep this list in case-insensitive alphabetical order. > */ > typedef struct AdapterInfo AdapterInfo; > [...] > > /* > * Pointer types > * Such typedefs should be limited to cases where the typedef's users > * are oblivious of its "pointer-ness". > * Please keep this list in case-insensitive alphabetical order. > */ > typedef struct IRQState *qemu_irq; > > /* > * Function types > */ > typedef void SaveStateHandler(QEMUFile *f, void *opaque); > typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id); > typedef void (*qemu_irq_handler)(void *opaque, int n, int level); > > What do you think? A definite improvement on what is currently there ;-) > > [...] -- Alex Bennée
Alex Bennée <alex.bennee@linaro.org> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Alex Bennée <alex.bennee@linaro.org> writes: >>> >>>> Markus Armbruster <armbru@redhat.com> writes: >>>> >>>>> While there, drop the obsolete file comment. >>>>> >>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>> --- >>>>> include/qemu/typedefs.h | 12 ++++++++---- >>>>> 1 file changed, 8 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >>>>> index fcdaae58c4..29346648d4 100644 >>>>> --- a/include/qemu/typedefs.h >>>>> +++ b/include/qemu/typedefs.h >>>>> @@ -1,10 +1,10 @@ >>>>> #ifndef QEMU_TYPEDEFS_H >>>>> #define QEMU_TYPEDEFS_H >>>>> >>>>> -/* A load of opaque types so that device init declarations don't have to >>>>> - pull in all the real definitions. */ >>>>> - >>>>> -/* Please keep this list in case-insensitive alphabetical order */ >>>>> +/* >>>>> + * Incomplete struct types >>>> >>>> Maybe expand this a little... >>>> >>>> "Incomplete struct types for modules that don't need the complete >>>> definitions but still pass around typed variables."? >>> >>> If we explain proper use of qemu/typedefs.h in HACKING, as discussed in >>> review of v1[*], we could point there. >> >> Perhaps rewriting the obsolete file comment would be better. Something >> like this: >> >> /* >> * This header is for selectively avoiding #include just to get a >> * typedef name. >> * >> * Declaring a typedef name in its "obvious" place can result in >> * inclusion cycles, in particular for complete struct and union >> * types that need more types for their members. It can also result >> * in headers pulling in many more headers, slowing down builds. >> * >> * You can break such cycles and unwanted dependencies by declaring >> * the typedef name here. >> * >> * For struct types used in only a few headers, judicious use of the >> * struct tag instead of the typedef name is commonly preferable. >> */ >> >> /* >> * Incomplete struct types >> * Please keep this list in case-insensitive alphabetical order. >> */ >> typedef struct AdapterInfo AdapterInfo; >> [...] >> >> /* >> * Pointer types >> * Such typedefs should be limited to cases where the typedef's users >> * are oblivious of its "pointer-ness". >> * Please keep this list in case-insensitive alphabetical order. >> */ >> typedef struct IRQState *qemu_irq; >> >> /* >> * Function types >> */ >> typedef void SaveStateHandler(QEMUFile *f, void *opaque); >> typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id); >> typedef void (*qemu_irq_handler)(void *opaque, int n, int level); >> >> What do you think? > > A definite improvement on what is currently there ;-) Amending my commit. Thanks!
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index fcdaae58c4..29346648d4 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -1,10 +1,10 @@ #ifndef QEMU_TYPEDEFS_H #define QEMU_TYPEDEFS_H -/* A load of opaque types so that device init declarations don't have to - pull in all the real definitions. */ - -/* Please keep this list in case-insensitive alphabetical order */ +/* + * Incomplete struct types + * Please keep this list in case-insensitive alphabetical order. + */ typedef struct AdapterInfo AdapterInfo; typedef struct AddressSpace AddressSpace; typedef struct AioContext AioContext; @@ -101,6 +101,10 @@ typedef struct SHPCDevice SHPCDevice; typedef struct SSIBus SSIBus; typedef struct VirtIODevice VirtIODevice; typedef struct Visitor Visitor; + +/* + * Function types + */ typedef void SaveStateHandler(QEMUFile *f, void *opaque); typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);