diff mbox

[v4,04/10] qapi: Emit implicit structs in generated C

Message ID 1457194595-16189-5-git-send-email-eblake@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Blake March 5, 2016, 4:16 p.m. UTC
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.

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.  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
single underscore, and we've already documented and enforce that
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 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.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v4: new patch
---
 scripts/qapi.py       |  3 +--
 scripts/qapi-types.py | 12 ++++++------
 scripts/qapi-visit.py | 18 ++++++++----------
 3 files changed, 15 insertions(+), 18 deletions(-)

Comments

Markus Armbruster March 8, 2016, 2:24 p.m. UTC | #1
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?

>                   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 <eblake@redhat.com>

Can't find fault with the patch itself.
Eric Blake March 8, 2016, 4:03 p.m. UTC | #2
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.
Markus Armbruster March 8, 2016, 7:09 p.m. UTC | #3
Eric Blake <eblake@redhat.com> writes:

> 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 used to be on the "narrow" side of the fence, but I've come to sit on
it.

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

What could be improved if we changed visit_object_type() to take a
QAPISchemaObjectType instead of name, info, base, members, variants?

I believe we'd leave gen_object() unchanged to keep
visit_alternate_type() simple.

Here's a different one: we could drop visit_object_type_flat().

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

For an array of T, we use TList.

For a simple union U's implicit enum, we use UKind.

Both predate the application of rational thought to clashes in generated
code ;)  Reserving these names was easier than changing them, so that's
what we did.

For implicit objects, we use :obj-NAME-ROLE.  We can change that, but
need to keep QAPISchemaMember._pretty_owner() working.

We could use c_name(q_obj_NAME_ROLE).

The resulting types aren't in CamelCase.  Making them CamelCase would be
easy enough when NAME is a type name (it is for ROLEs wrapper and base),
but for command and event names (ROLE arg) it involves playing stupid
games with case, dash and underscore.  Let's not go there, and pray the
coding style police looks the other way.

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

Hmm.

I feel awful generating >100KiB of code that gets included pretty much
every time we compile anything.  Perhaps the proper fix for that is to
find out *why* we include qapi-types.h so much, then figure out how to
include it much, much less.

Here's a guilty one: error.h.  I believe it includes qapi-types.h just
for QapiErrorClass.  I guess it's only in the QAPI schema to have
QapiErrorClass_lookup[] generated.  I'd gladly maintain those nine(!)
lines of code manually if it helps me drop >100KiB of useless code from
many (most?) compiles.

Another one are builtin QAPI types.  A few headers want them.  We could
put them into a separate header instead of generating them into
qapi-types.h.  Could also enable getting rid of the ugly "spew builtins
into every header, but guard them with #ifdeffery" trick.
Eric Blake March 9, 2016, 5:42 a.m. UTC | #4
On 03/08/2016 12:09 PM, Markus Armbruster wrote:

> 
>> 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.
> 
> What could be improved if we changed visit_object_type() to take a
> QAPISchemaObjectType instead of name, info, base, members, variants?
> 
> I believe we'd leave gen_object() unchanged to keep
> visit_alternate_type() simple.
> 
> Here's a different one: we could drop visit_object_type_flat().

Indeed. But I'd rather get v5 of this series out sooner rather than
later, so I'll save the change for a later day.


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

Seems doable. We already reserved q_ for ourselves (commit 9fb081e0), so
far using it mainly by c_name.  There's still the risk that c_name adds
q_ onto something ticklish which then clashes with our use of q_ on an
implicit type for something else; except that we haven't declared 'obj'
as ticklish.

> I feel awful generating >100KiB of code that gets included pretty much
> every time we compile anything.  Perhaps the proper fix for that is to
> find out *why* we include qapi-types.h so much, then figure out how to
> include it much, much less.
> 
> Here's a guilty one: error.h.  I believe it includes qapi-types.h just
> for QapiErrorClass.  I guess it's only in the QAPI schema to have
> QapiErrorClass_lookup[] generated.  I'd gladly maintain those nine(!)
> lines of code manually if it helps me drop >100KiB of useless code from
> many (most?) compiles.

Commit 13f59ae predates my work on qapi, but I think you're right that
error.h including qapi-types.h is a big offender and appears to do so
solely for ErrorClass.  But how about as a followup series, so that v5
gets out the door sooner.

> 
> Another one are builtin QAPI types.  A few headers want them.  We could
> put them into a separate header instead of generating them into
> qapi-types.h.  Could also enable getting rid of the ugly "spew builtins
> into every header, but guard them with #ifdeffery" trick.

In that same series.
Markus Armbruster March 9, 2016, 7:23 a.m. UTC | #5
Eric Blake <eblake@redhat.com> writes:

> On 03/08/2016 12:09 PM, Markus Armbruster wrote:
>
>> 
>>> 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.
>> 
>> What could be improved if we changed visit_object_type() to take a
>> QAPISchemaObjectType instead of name, info, base, members, variants?
>> 
>> I believe we'd leave gen_object() unchanged to keep
>> visit_alternate_type() simple.
>> 
>> Here's a different one: we could drop visit_object_type_flat().
>
> Indeed. But I'd rather get v5 of this series out sooner rather than
> later, so I'll save the change for a later day.

I agree that we've wandered beyond the scope of this series here.  It's
not about the visitor, it merely struggles with the visitor's narrow
view on the visited objects.  That led us to discuss whether that view
still makes sense.  We're not sure.

>>>> 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.
>
> Seems doable. We already reserved q_ for ourselves (commit 9fb081e0), so
> far using it mainly by c_name.  There's still the risk that c_name adds
> q_ onto something ticklish which then clashes with our use of q_ on an
> implicit type for something else; except that we haven't declared 'obj'
> as ticklish.

As far as I can tell, c_name() is still the only user of q_.  If we add
more, we need to update the "Reserve the entire 'q_' namespace for
c_name()" comment, and we should try to keep track of our use of q_
somehow.

Alternatively, reserve another chunk of the name space for implicit
object types.  Might be easier to maintain.

>> I feel awful generating >100KiB of code that gets included pretty much
>> every time we compile anything.  Perhaps the proper fix for that is to
>> find out *why* we include qapi-types.h so much, then figure out how to
>> include it much, much less.
>> 
>> Here's a guilty one: error.h.  I believe it includes qapi-types.h just
>> for QapiErrorClass.  I guess it's only in the QAPI schema to have
>> QapiErrorClass_lookup[] generated.  I'd gladly maintain those nine(!)
>> lines of code manually if it helps me drop >100KiB of useless code from
>> many (most?) compiles.
>
> Commit 13f59ae predates my work on qapi, but I think you're right that
> error.h including qapi-types.h is a big offender and appears to do so
> solely for ErrorClass.  But how about as a followup series, so that v5
> gets out the door sooner.

Teaching error.h manners doesn't depend on anything in your pipe.  I'm
invoking "Error Reporting" maintainer privileges: the task is mine :)

>> Another one are builtin QAPI types.  A few headers want them.  We could
>> put them into a separate header instead of generating them into
>> qapi-types.h.  Could also enable getting rid of the ugly "spew builtins
>> into every header, but guard them with #ifdeffery" trick.
>
> In that same series.

I'd expect this to be a series of its own, going through the QAPI tree.
diff mbox

Patch

diff --git a/scripts/qapi.py b/scripts/qapi.py
index b1b87ee..6cf0a75 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -999,7 +999,6 @@  class QAPISchemaObjectType(QAPISchemaType):
         return self.name[0] == ':'

     def c_name(self):
-        assert not self.is_implicit()
         return QAPISchemaType.c_name(self)

     def c_type(self):
@@ -1476,7 +1475,7 @@  def c_enum_const(type_name, const_name, prefix=None):
         type_name = prefix
     return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()

-c_name_trans = string.maketrans('.-', '__')
+c_name_trans = string.maketrans('.-:', '___')


 # Map @name to a valid C identifier.
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index f194bea..fa2d6af 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -61,8 +61,7 @@  def gen_object(name, base, members, variants):
     ret = ''
     if variants:
         for v in variants.variants:
-            if (isinstance(v.type, QAPISchemaObjectType) and
-                    not v.type.is_implicit()):
+            if isinstance(v.type, QAPISchemaObjectType):
                 ret += gen_object(v.type.name, v.type.base,
                                   v.type.local_members, v.type.variants)

@@ -197,9 +196,8 @@  class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         self._btin = None

     def visit_needed(self, entity):
-        # Visit everything except implicit objects
-        return not (entity.is_implicit() and
-                    isinstance(entity, QAPISchemaObjectType))
+        # Visit everything except the special :empty builtin
+        return entity.name != ':empty'

     def _gen_type_cleanup(self, name):
         self.decl += gen_type_cleanup_decl(name)
@@ -233,7 +231,9 @@  class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         self.decl += gen_object(name, base, members, variants)
         if base:
             self.decl += gen_upcast(name, base)
-        self._gen_type_cleanup(name)
+        if name[0] != ':':
+            # implicit types won't be directly allocated/freed
+            self._gen_type_cleanup(name)

     def visit_alternate_type(self, name, info, variants):
         self._fwdecl += gen_fwd_object_or_array(name)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a712e9a..b4303a7 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -215,13 +215,11 @@  out:


 def gen_visit_object(name, base, members, variants):
-    ret = gen_visit_object_members(name, base, members, variants)
-
     # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
     # *obj, but then visit_type_FOO_members() fails, we should clean up *obj
     # rather than leaving it non-NULL. As currently written, the caller must
     # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
-    ret += mcgen('''
+    return mcgen('''

 void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
 {
@@ -245,8 +243,6 @@  out:
 ''',
                  c_name=c_name(name))

-    return ret
-

 class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
     def __init__(self):
@@ -269,9 +265,8 @@  class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
         self._btin = None

     def visit_needed(self, entity):
-        # Visit everything except implicit objects
-        return not (entity.is_implicit() and
-                    isinstance(entity, QAPISchemaObjectType))
+        # Visit everything except the special :empty builtin
+        return entity.name != ':empty'

     def visit_enum_type(self, name, info, values, prefix):
         # Special case for our lone builtin enum type
@@ -297,8 +292,11 @@  class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):

     def visit_object_type(self, name, info, base, members, variants):
         self.decl += gen_visit_members_decl(name)
-        self.decl += gen_visit_decl(name)
-        self.defn += gen_visit_object(name, base, members, variants)
+        self.defn += gen_visit_object_members(name, base, members, variants)
+        if name[0] != ':':
+            # only explicit types need an allocating visit
+            self.decl += gen_visit_decl(name)
+            self.defn += gen_visit_object(name, base, members, variants)

     def visit_alternate_type(self, name, info, variants):
         self.decl += gen_visit_decl(name)