| Message ID | 20210729175554.686474-7-ehabkost@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QOM documentation updates | expand |
Eduardo Habkost <ehabkost@redhat.com> writes: > When there's no specific class struct used for a QOM type, we > normally don't define a typedef for it. Remove the typedef from > the minimal example, as it is unnecessary. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > docs/devel/qom.rst | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst > index 05d045bf570..dee60a64c0a 100644 > --- a/docs/devel/qom.rst > +++ b/docs/devel/qom.rst > @@ -20,9 +20,6 @@ features: > > #define TYPE_MY_DEVICE "my-device" > > - // No new virtual functions: we can reuse the typedef for the > - // superclass. > - typedef DeviceClass MyDeviceClass; > typedef struct MyDevice > { > DeviceState parent; Documenting existing practice makes sense, but I'm not sure the existing practice to elide this typedef makes sense.
On Fri, 30 Jul 2021 at 09:18, Markus Armbruster <armbru@redhat.com> wrote: > > Eduardo Habkost <ehabkost@redhat.com> writes: > > > When there's no specific class struct used for a QOM type, we > > normally don't define a typedef for it. Remove the typedef from > > the minimal example, as it is unnecessary. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > docs/devel/qom.rst | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst > > index 05d045bf570..dee60a64c0a 100644 > > --- a/docs/devel/qom.rst > > +++ b/docs/devel/qom.rst > > @@ -20,9 +20,6 @@ features: > > > > #define TYPE_MY_DEVICE "my-device" > > > > - // No new virtual functions: we can reuse the typedef for the > > - // superclass. > > - typedef DeviceClass MyDeviceClass; > > typedef struct MyDevice > > { > > DeviceState parent; > > Documenting existing practice makes sense, but I'm not sure the existing > practice to elide this typedef makes sense. The QOMConventions page on the wiki https://wiki.qemu.org/Documentation/QOMConventions makes what I think is a reasonable distinction: # If your class (a) will be subclassed or (b) has member fields it needs # to put in its class struct then you should write all of: # # a FOO_CLASS macro # a FOO_GET_CLASS macro # a FooClass structure definition containing at least the parent class field # # and your TypeInfo for this class should set the .class_size field to sizeof(FooClass). # # These ensure that nothing in future should need changing if new fields are # added to your class struct, and that any subclasses have the correct typenames # available so they won't need to change either even if your implementation changes. # # If your class meets neither of the above requirements (ie it is a simple leaf class) then: # # don't provide FOO_CLASS or FOO_GET_CLASS # don't provide a FooClass structure # leave the TypeInfo's .class_size field unset. # # If a change means a class which didn't provide these macros/types now needs to # provide them, then your change should add all of them (ie move the class from # the latter category to the former). By those principles, we should never do "typedef DeviceClass MyDeviceClass" -- either we have a real MyDeviceClass which contains at least the parent class field (ie is not a mere typedef), or we don't provide MyDeviceClass at all. I would say the rationale for the wiki's distinction is that we don't want to require unnecessary boilerplate for leaf classes without methods (which are by far the most common kind of class), but we don't want a free-for-all regarding how you write things either. So we define a standard pattern for leaves and a standard pattern for everything else. -- PMM
On Fri, Jul 30, 2021 at 09:43:55AM +0100, Peter Maydell wrote: > On Fri, 30 Jul 2021 at 09:18, Markus Armbruster <armbru@redhat.com> wrote: > > > > Eduardo Habkost <ehabkost@redhat.com> writes: > > > > > When there's no specific class struct used for a QOM type, we > > > normally don't define a typedef for it. Remove the typedef from > > > the minimal example, as it is unnecessary. > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > docs/devel/qom.rst | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst > > > index 05d045bf570..dee60a64c0a 100644 > > > --- a/docs/devel/qom.rst > > > +++ b/docs/devel/qom.rst > > > @@ -20,9 +20,6 @@ features: > > > > > > #define TYPE_MY_DEVICE "my-device" > > > > > > - // No new virtual functions: we can reuse the typedef for the > > > - // superclass. > > > - typedef DeviceClass MyDeviceClass; > > > typedef struct MyDevice > > > { > > > DeviceState parent; > > > > Documenting existing practice makes sense, but I'm not sure the existing > > practice to elide this typedef makes sense. > > The QOMConventions page on the wiki > https://wiki.qemu.org/Documentation/QOMConventions > makes what I think is a reasonable distinction: > > # If your class (a) will be subclassed or (b) has member fields it needs > # to put in its class struct then you should write all of: > # > # a FOO_CLASS macro > # a FOO_GET_CLASS macro > # a FooClass structure definition containing at least the parent class field > # > # and your TypeInfo for this class should set the .class_size field to > sizeof(FooClass). > # > # These ensure that nothing in future should need changing if new fields are > # added to your class struct, and that any subclasses have the correct typenames > # available so they won't need to change either even if your > implementation changes. > # > # If your class meets neither of the above requirements (ie it is a > simple leaf class) then: > # > # don't provide FOO_CLASS or FOO_GET_CLASS > # don't provide a FooClass structure > # leave the TypeInfo's .class_size field unset. > # > # If a change means a class which didn't provide these macros/types now needs to > # provide them, then your change should add all of them (ie move the class from > # the latter category to the former). > > By those principles, we should never do "typedef DeviceClass MyDeviceClass" -- > either we have a real MyDeviceClass which contains at least the parent > class field (ie is not a mere typedef), or we don't provide MyDeviceClass > at all. > > I would say the rationale for the wiki's distinction is that we don't > want to require unnecessary boilerplate for leaf classes without > methods (which are by far the most common kind of class), but we don't > want a free-for-all regarding how you write things either. So we define > a standard pattern for leaves and a standard pattern for everything else. Neither the wiki page above nor this part of the docs is really showing best practice. The best practice is much later in this doc where it shows the macros that eliminate all of tedious boilerplate code: https://qemu-project.gitlab.io/qemu/devel/qom.html#standard-type-declaration-and-definition-macros Personally I'd suggest we reverse the documentation order, so that the first thing we tell people about at the simple macros for declaring types, and strongly recomend that they are used. Only document the manual boilerplate at the end, merely as reference material, and recommend against its use. Regards, Daniel
On Thu, 29 Jul 2021 at 19:01, Eduardo Habkost <ehabkost@redhat.com> wrote: > > When there's no specific class struct used for a QOM type, we > normally don't define a typedef for it. Remove the typedef from > the minimal example, as it is unnecessary. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > docs/devel/qom.rst | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst > index 05d045bf570..dee60a64c0a 100644 > --- a/docs/devel/qom.rst > +++ b/docs/devel/qom.rst > @@ -20,9 +20,6 @@ features: > > #define TYPE_MY_DEVICE "my-device" > > - // No new virtual functions: we can reuse the typedef for the > - // superclass. > - typedef DeviceClass MyDeviceClass; > typedef struct MyDevice > { > DeviceState parent; Reviewed-by: Peter Maydell <peter.maydell@linaro.org> though I agree with Daniel that in the long-term we should reverse the structure of the documents so the recommended macros go first and the behind-the-scenes boilerplate last. thanks -- PMM
On Mon, Aug 02, 2021 at 01:19:14PM +0100, Peter Maydell wrote: > On Thu, 29 Jul 2021 at 19:01, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > When there's no specific class struct used for a QOM type, we > > normally don't define a typedef for it. Remove the typedef from > > the minimal example, as it is unnecessary. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > docs/devel/qom.rst | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst > > index 05d045bf570..dee60a64c0a 100644 > > --- a/docs/devel/qom.rst > > +++ b/docs/devel/qom.rst > > @@ -20,9 +20,6 @@ features: > > > > #define TYPE_MY_DEVICE "my-device" > > > > - // No new virtual functions: we can reuse the typedef for the > > - // superclass. > > - typedef DeviceClass MyDeviceClass; > > typedef struct MyDevice > > { > > DeviceState parent; > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > though I agree with Daniel that in the long-term we should reverse > the structure of the documents so the recommended macros go first > and the behind-the-scenes boilerplate last. I agree 100%, and maybe I will give it a try later. My immediate goal was to just remove the examples in the docs where type checking macros were defined manually. Since we introduced the new QOM helper macros, the number of OBJECT_CHECK macros in the QEMU tree almost doubled (from 40 in commit 8110fa1d94f2 to 75 in commit bccabb3a5d60).
On Wed, 4 Aug 2021 at 21:40, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Aug 02, 2021 at 01:19:14PM +0100, Peter Maydell wrote: > > On Thu, 29 Jul 2021 at 19:01, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > When there's no specific class struct used for a QOM type, we > > > normally don't define a typedef for it. Remove the typedef from > > > the minimal example, as it is unnecessary. > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > > > though I agree with Daniel that in the long-term we should reverse > > the structure of the documents so the recommended macros go first > > and the behind-the-scenes boilerplate last. > > I agree 100%, and maybe I will give it a try later. > > My immediate goal was to just remove the examples in the docs > where type checking macros were defined manually. Since we > introduced the new QOM helper macros, the number of OBJECT_CHECK > macros in the QEMU tree almost doubled (from 40 in commit > 8110fa1d94f2 to 75 in commit bccabb3a5d60). Yeah, to be clear, I'm all in favour of changes like this patchset and don't think we should hold them up because we have a theoretical larger restructuring of the documentation we might one day (or never) get round to. -- PMM
diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst index 05d045bf570..dee60a64c0a 100644 --- a/docs/devel/qom.rst +++ b/docs/devel/qom.rst @@ -20,9 +20,6 @@ features: #define TYPE_MY_DEVICE "my-device" - // No new virtual functions: we can reuse the typedef for the - // superclass. - typedef DeviceClass MyDeviceClass; typedef struct MyDevice { DeviceState parent;
When there's no specific class struct used for a QOM type, we normally don't define a typedef for it. Remove the typedef from the minimal example, as it is unnecessary. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- docs/devel/qom.rst | 3 --- 1 file changed, 3 deletions(-)