Re: [Qemu-devel] [PATCH v3 15/50] qapi-types: refactor variants handling

[Marc-André Lureau]

Hi

On Thu, Dec 7, 2017 at 4:57 PM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> Generate variants objects outside gen_object(). This will allow to
>> easily wrap gen_object() with ifcond_decorator in the following patch.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  scripts/qapi-types.py | 37 +++++++++++++++++++++++--------------
>>  1 file changed, 23 insertions(+), 14 deletions(-)
>>
>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> index 915786c463..2b3588267b 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -53,23 +53,27 @@ def gen_struct_members(members):
>>      return ret
>>
>>
>> -def gen_object(name, base, members, variants):
>> -    if name in objects_seen:
>> -        return ''
>> -    objects_seen.add(name)
>> -
>> +def gen_variants_objects(variants):
>>      ret = ''
>>      if variants:
>>          for v in variants.variants:
>>              if isinstance(v.type, QAPISchemaObjectType):
>> +                ret += gen_variants_objects(v.type.variants)
>>                  ret += gen_object(v.type.name, v.type.base,
>>                                    v.type.local_members, v.type.variants)
>> +    return ret
>>
>> -    ret += mcgen('''
>> +
>> +def gen_object(name, base, members, variants):
>> +    if name in objects_seen:
>> +        return ''
>> +    objects_seen.add(name)
>> +
>> +    ret = mcgen('''
>>
>>  struct %(c_name)s {
>>  ''',
>> -                 c_name=c_name(name))
>> +                c_name=c_name(name))
>>
>>      if base:
>>          if not base.is_implicit():
>> @@ -218,11 +222,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>>              self.decl += gen_array(name, element_type)
>>              self._gen_type_cleanup(name)
>>
>> -    def visit_object_type(self, name, info, ifcond, base, members, 
>> variants):
>> -        # Nothing to do for the special empty builtin
>> -        if name == 'q_empty':
>> -            return
>> -        self._fwdecl += gen_fwd_object_or_array(name)
>> +    def _gen_object(self, name, info, ifcond, base, members, variants):
>>          self.decl += gen_object(name, base, members, variants)
>>          if base and not base.is_implicit():
>>              self.decl += gen_upcast(name, base)
>> @@ -232,10 +232,19 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>>              # implicit types won't be directly allocated/freed
>>              self._gen_type_cleanup(name)
>>
>> +    def visit_object_type(self, name, info, ifcond, base, members, 
>> variants):
>> +        # Nothing to do for the special empty builtin
>> +        if name == 'q_empty':
>> +            return
>> +        self._fwdecl += gen_fwd_object_or_array(name)
>> +        self.decl += gen_variants_objects(variants)
>> +        self._gen_object(name, info, None, base, members, variants)
>> +
>>      def visit_alternate_type(self, name, info, ifcond, variants):
>>          self._fwdecl += gen_fwd_object_or_array(name)
>> -        self.decl += gen_object(name, None, [variants.tag_member], variants)
>> -        self._gen_type_cleanup(name)
>> +        self.decl += gen_variants_objects(variants)
>> +        self._gen_object(name, info, None, None,
>> +                         [variants.tag_member], variants)
>
> Where did self._gen_type_cleanup(name) go?  Hmm, I guess it's now in
> _gen_object().  Confusing.

This factors out common code that must be wrap by the same ifcond, in
the following patch.

>>
>>  # If you link code generated from multiple schemata, you want only one
>>  # instance of the code for built-in types.  Generate it only when
>
> If I read this patch correctly, you move code from the beginning of
> gen_object() to new gen_variants_objects(), then arrange to have
> gen_variants_objects() called right before gen_object().  Correct?
>
> In old gen_object(), the code is guarded by
>
>        if name in objects_seen:
>            return ''
>        objects_seen.add(name)
>
> In new gen_variants_objects(), it isn't.  Why?

gen_variants_objects() calls gen_object() for each variants, so it
remains guarded.



-- 
Marc-André Lureau