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

[Markus Armbruster]

Eric Blake <address@hidden> 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?

>                   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.

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?

> Signed-off-by: Eric Blake <address@hidden>

Can't find fault with the patch itself.