Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> Fixes flat unions to get the base's base members.  Test case is from
>> commit 2fc0043, in qapi-schema-test.json:
>> 
>
> Okay, I see a cause for part of my confusion.
>
>>  
>> +class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>
>> +    def visit_end(self):
>> +        self.decl = self.fwdecl + self.decl
>> +        self.fwdecl = None
>> +        self.defn = self.fwdefn + self.defn
>> +        self.fwdefn = None
>> +        # To avoid header dependency hell, we always generate
>> +        # declarations for built-in types in our header files and
>> +        # simply guard them.
>> +        self.btin += guardend('QAPI_TYPES_BUILTIN')
>> +        self.decl = self.btin + self.decl
>> +        self.btin = None
>
> The new code goes to great lengths to separate all builtin decls up
> front.  But...

Because they all need to be under the QAPI_TYPES_BUILTIN guard.  If I
didn't separate them, each of them would need its own guard, which would
be ugly.

>> +        # Doesn't work for cases where we link in multiple objects
>> +        # that have the functions defined, so generate them only with
>> +        # option -b (do_builtins).
>> +    def _gen_type_cleanup(self, name):
>> +        self.decl += generate_type_cleanup_decl(name)
>> +        self.defn += generate_type_cleanup(name)
>> +    def visit_enum_type(self, name, info, values):
>> +        self.fwdecl += generate_enum(name, values)
>> +        self.fwdefn += generate_enum_lookup(name, values)
>> +    def visit_array_type(self, name, info, element_type):
>> +        if isinstance(element_type, QAPISchemaBuiltinType):
>> +            self.btin += gen_fwd_object_or_array(name)
>> +            self.btin += gen_array(name, element_type)
>> +            self.btin += generate_type_cleanup_decl(name)
>> +            if do_builtins:
>> +                self.defn += generate_type_cleanup(name)
>
> ...it is still interleaving builtin defns with everything else.

Because there's no such need for the builtin definitions.

>> -exprs = QAPISchema(input_file).get_exprs()
>> -
>> -fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
>> -for typename in builtin_types.keys():
>> -    fdecl.write(generate_fwd_builtin(typename))
>> -fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
>
> Meanwhile, the old code did builtin intList definitions up front...
>
>> -
>> -for expr in exprs:
>> -    ret = ""
>> -    if expr.has_key('struct'):
>
>> -# to avoid header dependency hell, we always generate declarations
>> -# for built-in types in our header files and simply guard them
>> -fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
>> -for typename in builtin_types.keys():
>> -    fdecl.write(generate_type_cleanup_decl(typename + "List"))
>> -fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
>
> ...but declared the cleanup functions at the end (so code motion 1 is
> that the cleanup functions are now interleaved with the intList
> declarations)...
>
>> -
>> -# ...this doesn't work for cases where we link in multiple objects that
>> -# have the functions defined, so we use -b option to provide control
>> -# over these cases
>> -if do_builtins:
>> -    for typename in builtin_types.keys():
>> -        fdef.write(generate_type_cleanup(typename + "List"))
>
> ...and wrote the definitions at the end (so code motion 2 is that the
> definitions are now interleaved with all other definitions).
>
> Attached a few quick hacks that reduce (but not eliminate) some of the
> visitor's churn to the generated files, by consolidating the location of
> the builtins and cleaning up struct declarations as separate cleanups
> (to be applied prior to 26/47), then updating the visitor to use the
> same builtin order (to be applied after or squashed with 26/47).
>
> It still doesn't solve everything, but with those hacks, I was able to
> get to a slightly more manageable:
>
>  qapi-types.c                        |  864 ++++----
>  qapi-types.h                        | 3602 ++++++++++++++++++------------------
>  qga/qapi-generated/qga-qapi-types.c |  110 -
>  qga/qapi-generated/qga-qapi-types.h |  405 ++--
>  4 files changed, 2585 insertions(+), 2396 deletions(-)

Still plenty bad...

> I'm sure there are further things that could be done, but at this point,
> I hope you get my picture, and I'll quit focusing on this particular patch.

We need to decide how much code churn to accept just for making the diff
of the generated code easier to review.

Thanks!