Re: [Qemu-devel] [PATCH v10 07/25] qapi-visit: Split off visit_type_FOO_fields forward decl

[Eric Blake]

On 10/23/2015 07:46 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> We generate a static visit_type_FOO_fields() for every type
>> FOO.  However, sometimes we need a forward declaration. Split
>> the code to generate the forward declaration out of
>> gen_visit_implicit_struct() into a new gen_visit_fields_decl(),
>> and also prepare for a forward declaration to be emitted
>> during gen_visit_struct(), so that a future patch can switch
>> from using visit_type_FOO_implicit() to the simpler
>> visit_type_FOO_fields() as part of unboxing the base class
>> of a struct.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>
>> ---
>> v10: new patch, split from 5/17
>> ---
>>  scripts/qapi-visit.py | 35 ++++++++++++++++++++++++-----------
>>  1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index e878018..7204ed0 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -15,7 +15,12 @@
>>  from qapi import *
>>  import re
>>
>> +# visit_type_FOO_implicit() is emitted as needed; track if it has already
>> +# been output. No forward declaration is needed.
>>  implicit_structs_seen = set()
> 
> I initially read this as "No forward is needed then", but that's wrong.
> Suggest to drop that sentence.

No forward declaration of visit_type_FOO_implicit() is ever needed. But
yes, dropping the sentence doesn't lose any information, and avoids
confusion.

> 
>> +
>> +# visit_type_FOO_fields() is always emitted; track if a forward declaration
>> +# or implementation has already been output.
>>  struct_fields_seen = set()
> 
> Yup.

Actually, I have plans for a later patch to only emit it for non-empty
structs (getting rid of no-op visit_type_Abort_fields() and friends), as
part of unifying it with gen_visit_union() (since unions don't have
local members, they also wouldn't get a visit_type_Union_fields) - but
not in this subset of the series.

>> @@ -62,13 +72,12 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, 
>> %(c_type)s **obj, Error *
>>
>>
>>  def gen_visit_struct_fields(name, base, members):
>> -    struct_fields_seen.add(name)
>> -
>>      ret = ''
>>
>>      if base:
>>          ret += gen_visit_implicit_struct(base)
>>
>> +    struct_fields_seen.add(name)
>>      ret += mcgen('''
>>
> 
> Minor cleanup not mentioned in commit message.  Okay.

Not minor, and I probably should mention it explicitly in the message. I
moved it to make sure that gen_visit_implicit_struct() properly emits a
forward declaration when necessary; we must not modify
struct_fields_seen any sooner than when the next thing in the output
stream is either the forward declaration or the implementation.

>> @@ -100,7 +109,11 @@ out:
>>
>>
>>  def gen_visit_struct(name, base, members):
>> -    ret = gen_visit_struct_fields(name, base, members)
>> +    ret = ''
>> +    if base:
>> +        ret += gen_visit_fields_decl(base)
>> +
>> +    ret += gen_visit_struct_fields(name, base, members)
>>
>>      # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
>>      # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
> 
> What's the purpose of this hunk?

Umm, no clue. Maybe to test that you are reviewing things closely? :)

Actually, I think I have a real answer: leftovers from rebasing.  Once I
fix gen_visit_union() to reuse visit_type_BASE_fields() (patch 11/25),
then gen_visit_struct_fields() no longer calls
gen_visit_implicit_struct(base), and had to replace it with a call to
gen_visit_fields_decl() somewhere.  And I didn't always have this patch
in this position of the series.

But for this patch, you are right that taking it out changes nothing at
this point (since gen_visit_struct_fields(, base) calls
gen_visit_implicit_struct(base) calls gen_visit_fields_decl(base)).

I'm testing if removing this hunk breaks anything later, and will either
post fixup patches or roll v11 at the end of v10 review (depends on how
many other findings you have).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature