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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

The proper place for the .add() is right next to where the thing it
tracks gets done, no argument.

I believe your cleanup makes an actual difference only when
gen_visit_implicit_struct(base) can call gen_visit_struct_fields(name).
Obviously impossible now, which is why I called it "minor".

PATCH 10 will drop the code between the old spot and the new spot.
Unless something between this patch and PATCH 10 depends on this
cleanup, I'd simply leave it dirty until then.

>>> @@ -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? :)

Passed the test, I guess ;)

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

Okay.