Re: [PATCH 01/14] include/hw: add helpers for defining versioned machine types

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, May 02, 2024 at 12:34:49PM +0200, Thomas Huth wrote:
> On 01/05/2024 20.27, Daniel P. Berrangé wrote:
> > The various targets which define versioned machine types have
> > a bunch of obfuscated macro code for defining unique function
> > and variable names using string concatenation.
> > 
> > This addes a couple of helpers to improve the clarity of such
> > code macro.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   include/hw/boards.h | 166 ++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 166 insertions(+)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 2fa800f11a..47ca450fca 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -414,6 +414,172 @@ struct MachineState {
> >       struct NumaState *numa_state;
> >   };
> > +/*
> > + * The macros which follow are intended to facilitate the
> > + * definition of versioned machine types, using a somewhat
> > + * similar pattern across targets:
> 
> I'd suggest to add a sentence at the end saying something like this: "For
> example, to create the macro for setting up a versioned "virt" machine could
> look like this:" (otherwise it's not immediately clear whether the example
> is about only the "macros which follow" or whether it's about how to set up
> a machine type)

Yes, will do

> 
> > + *  #define DEFINE_VIRT_MACHINE_IMPL(latest, ...) \
> > + *      static void MACHINE_VER_SYM(class_init, virt, __VA_ARGS__)( \
> > + *          ObjectClass *oc, \
> > + *          void *data) \

> > + *  #define DEFINE_VIRT_MACHINE_TAGGED(major, minor, micro, tag) \
> > + *      DEFINE_VIRT_MACHINE_IMPL(false, major, minor, micro, _, tag)
> > + */
> 
> I'd suggest to add a separate comment for the macro below, explaining that
> it is supposed to be used with __VA_ARGS__ to pick a certain other macro
> depending on the amount of entries in __VA_ARGS__.

Yep, Eric had the same suggestion

> 
> > +#define _MACHINE_VER_PICK(x1, x2, x3, x4, x5, x6, ...) x6
> > +
> > +/*
> > + * Construct a human targetted machine version string.
> 
> s/targetted/targeted/ according to my spell checker ?
> 
> Apart from the nits:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
>  Thomas
> 

With 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 :|