On Tue, May 02, 2017 at 04:29:32PM -0500, Eric Blake wrote:
> 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.
Yep. But I don't see a problem with using uint32_t or a simple
int.
>
> > +++ 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.)
I can do that in v2.
>
> > 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.
I will probably do this as a single QEMU_BUILD_BUG_ON near
visit_start_alternate().
>
> > +++ 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.
I am more used to see this written as "if (s & (1UL << n))" than
as "if (extract32(s, n, 1))", so I'm not sure.
I see some extract32(..., ..., 1) cases in the tree, so it's not
as unusual as I thought. I will probably give it a try.
>
> The idea makes sense, but I'm still not necessarily sold on using a long.
Thanks!
--
Eduardo
