Re: [PATCH for-6.2 06/10] docs: qom: Remove unnecessary class typedefs from example

[Daniel P . Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|