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é <berra...@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 <th...@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 :|
