On 05/02/2017 03:31 PM, Eduardo Habkost wrote: > This will allow visitors to make decisions based on the supported qtypes > of a given alternate type. The new parameter can replace the old > 'promote_int' argument, as qobject-input-visitor can simply check if > QTYPE_QINT is set in supported_qtypes. > > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> > ---
> @@ -416,7 +417,7 @@ void visit_end_list(Visitor *v, void **list);
> */
> void visit_start_alternate(Visitor *v, const char *name,
> GenericAlternate **obj, size_t size,
> - bool promote_int, Error **errp);
> + unsigned long supported_qtypes, Error **errp);
Why unsigned long (which is platform-dependent in size)? At the moment,
even unsigned char happens to be long enough, although I probably would
have used uint32_t.
Oh, I see, it's because you use the BIT() macros from bitops.h, which
are hardcoded to unsigned long.
> +++ b/scripts/qapi-visit.py
> @@ -161,20 +161,21 @@ void visit_type_%(c_name)s(Visitor *v, const char
> *name, %(c_name)s *obj, Error
>
>
> def gen_visit_alternate(name, variants):
> - promote_int = 'true'
> + qtypes = ['BIT(%s)' % (var.type.alternate_qtype())
> + for var in variants.variants]
> + supported_qtypes = '|'.join(qtypes)
Do you want ' | '.join(qtypes), so that at least the generated code
still follows recommended operator spacing? (The line is long no matter
what, though, and that's not worth worrying about.)
> ret = ''
> - for var in variants.variants:
> - if var.type.alternate_qtype() == 'QTYPE_QINT':
> - promote_int = 'false'
>
> ret += mcgen('''
>
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
> Error **errp)
> {
> Error *err = NULL;
> + unsigned long supported_qtypes = %(supported_qtypes)s;
>
> + assert(QTYPE__MAX < BITS_PER_LONG);
Do we really have to generate a separate copy of this assert in every
generated function? Especially when we know it is true by construction,
that seems like a lot. Having the assertion once in a .c file rather
than generated in multiple functions might be acceptable, though.
> +++ b/qapi/qobject-input-visitor.c
> @@ -349,7 +351,7 @@ static void qobject_input_start_alternate(Visitor *v,
> const char *name,
> }
> *obj = g_malloc0(size);
> (*obj)->type = qobject_type(qobj);
> - if (promote_int && (*obj)->type == QTYPE_QINT) {
> + if (!(supported_qtypes & BIT(QTYPE_QINT)) && (*obj)->type == QTYPE_QINT)
> {
Experimenting, does this read any better:
if (!extract32(supported_qtypes, QTYPE_QINT, 1) && ...
which would be another argument for uint32_t instead of unsigned long in
the signature.
The idea makes sense, but I'm still not necessarily sold on using a long.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
