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

[Markus Armbruster]

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