Re: [Qemu-devel] [PATCH v4 04/10] qapi: Emit implicit structs in generated C

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5651 bytes --]

On 03/08/2016 07:24 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> We already have several places that want to visit all the members
>> of an implicit object within a larger context (simple union variant,
>> event with anonymous data, command with anonymous arguments struct);
>> and will be adding another one soon (the ability to declare an
>> anonymous base for a flat union).  Having a C struct declared for
>> these implicit types, along with a visit_type_FOO_members() helper
>> function, will make for fewer special cases in our generator.
> 
> Yes.
> 
>> We do not, however, need qapi_free_FOO() or visit_type_FOO()
>> functions for implicit types, because they should not be used
>> directly outside of the generated code.  This is done by adding a
>> conditional in visit_object_type() for both qapi-types.py and
>> qapi-visit.py based on the object name.  The comparison of
>> "name[0] == ':'" feels a bit hacky, but beats changing the
>> signature of the visit_object_type() callback to pass a new
>> 'implicit' flag.
> 
> PRO: it matches what QAPISchemaObjectType.is_implicit() does.
> 
> CON: it duplicates what QAPISchemaObjectType.is_implicit() does.
> 
> Ways to adhere to DRY:
> 
> (1) Add a flag to visit_object_type() and, for consistency, to
>     visit_object_type_flat().
> 
> (2) Change the QAPISchemaVisitor.visit_FOO() to take a full FOO instead
>     of FOO.name, FOO.info and selected other members.
> 
> We've discussed (2) elsewhere.  The QAPISchemaEntity.visit() shrink to a
> mere double-dispatch.  The QAPISchemaVisitor gain full access to the
> things they visit.  The interface between the generators and the
> internal representation changes from a narrow, explicit and inflexible
> one to a much wider "anything goes" one.  Both the narrow and the wide
> interface have advantages and disadvantages.  Have we outgrown the
> narrow one?

Your argument that the narrow view forces us to think about things may
still hold.  I'm border-line on whether the switch is worth it, vs.
adding flags as the visitors want to learn more and more about what they
are visiting without having the actual Python object in hand.

I think what would sway me over the fence is looking at some of our
constructs: for example, qapi-types.py has gen_object() which it now
calls recursively.  When called directly from visit_object_type(), we
have all the pieces; when called from visit_alternate_type(), we have to
create a 1-element members array; and when called recursively, we have
to manually explode base into the four pieces.

> 
>>                   Also, now that we WANT to output C code for
>> implicits, we have to change the condition in the visit_needed()
>> filter.
>>
>> We have to special-case ':empty' as something that does not get
>> output: because it is a built-in type, it would be emitted in
>> multiple files (the main qapi-types.h and in qga-qapi-types.h)
>> causing compilation failure due to redefinition.  But since it
>> has no members, it's easier to just avoid an attempt to visit
>> that particular type.
>>
>> The patch relies on the fact that all our implicit objects start
>> with a leading ':', which can be transliteratated to a leading
> 
> transliterated
> 
>> single underscore, and we've already documented and enforce that
> 
> Uh, these are "always reserved for use as identifiers with file scope"
> (C99 7.1.3).  I'm afraid we need to use the q_ prefix.
> 
>> the user can't create QAPI names with a leading underscore, so
>> exposing the types won't create any collisions.  It is a bit
>> unfortunate that the generated struct names don't match normal
>> naming conventions, but it's not too bad, since they will only
>> be used in generated code.
> 
> The problem is self-inflicted: we make up these names in
> _make_implicit_object_type().  We could just as well make up names that
> come out prettier.

Any suggestions?  It seems easy enough to change, if we have something
that we like.

> 
> In fact, my first versions of the code had names starting with ':' for
> *all* implicit types.  I abandoned that for enum and array types when I
> realized I need C names for them, and the easiest way to get them making
> up names so that a trivial .c_name() works.  We can do the same for
> object types.
> 
>> The generated code grows substantially in size; in part because
>> it was easier to output every implicit type rather than adding
>> generator complexity to try and only output types as needed.
> 
> I happily trade larger .c files the optimizer can reduce for a simpler
> generator.  I'm less sanguine about larger .h files when they get
> included a lot.  qapi-types.h gets included basically everywhere, and
> grows from ~4000 to ~5250 lines.  How much of this is avoidable?

Not much: remember, because we use unboxed types in unions, all -wrapper
structs have to be present in the .h for the compiler to not complain
about incomplete types.

For -arg types (and for upcoming -base types in 8/10), we could get away
with only forward declarations of the type in the .h: the
visit_type_ARG_members() function uses only a pointer so it can live
with an incomplete type in the header and a complete type in a private
helper file or in the .c.  But we have fewer -arg classes in comparison
to the -wrapper classes, which means splitting on those lines would add
a lot of complexity to the generator for very little .h savings.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]