| Message ID | 1445576998-2921-8-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > > --- > 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. > + > +# visit_type_FOO_fields() is always emitted; track if a forward declaration > +# or implementation has already been output. > struct_fields_seen = set() Yup. > @@ -29,19 +34,24 @@ void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const char *name, Error ** > c_name=c_name(name), c_type=c_type) > > > +def gen_visit_fields_decl(typ): > + ret = '' > + if typ.name not in struct_fields_seen: > + ret += mcgen(''' > + > +static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp); > +''', > + c_type=typ.c_name()) > + struct_fields_seen.add(typ.name) > + return ret > + > + > def gen_visit_implicit_struct(typ): > if typ in implicit_structs_seen: > return '' > implicit_structs_seen.add(typ) > > - ret = '' > - if typ.name not in struct_fields_seen: > - # Need a forward declaration > - ret += mcgen(''' > - > -static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp); > -''', > - c_type=typ.c_name()) > + ret = gen_visit_fields_decl(typ) > > ret += mcgen(''' > > @@ -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. > static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp) > @@ -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?
On 10/23/2015 07:46 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> >> >> --- >> 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> writes: > On 10/23/2015 07:46 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> >>> >>> --- >>> 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.
On 10/23/2015 12:05 PM, Markus Armbruster wrote: >>>> 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". Oh, I was probably confusing that the implicit struct is for a different type (base) than what we are adding to struct_fields_seen (name). > > 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. Indeed, dropping this hunk and letting subsequent cleanup do it right makes no difference. Will add it to my fixup queue.
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() + +# visit_type_FOO_fields() is always emitted; track if a forward declaration +# or implementation has already been output. struct_fields_seen = set() @@ -29,19 +34,24 @@ void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const char *name, Error ** c_name=c_name(name), c_type=c_type) +def gen_visit_fields_decl(typ): + ret = '' + if typ.name not in struct_fields_seen: + ret += mcgen(''' + +static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp); +''', + c_type=typ.c_name()) + struct_fields_seen.add(typ.name) + return ret + + def gen_visit_implicit_struct(typ): if typ in implicit_structs_seen: return '' implicit_structs_seen.add(typ) - ret = '' - if typ.name not in struct_fields_seen: - # Need a forward declaration - ret += mcgen(''' - -static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp); -''', - c_type=typ.c_name()) + ret = gen_visit_fields_decl(typ) ret += mcgen(''' @@ -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(''' static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp) @@ -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
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 <eblake@redhat.com> --- v10: new patch, split from 5/17 --- scripts/qapi-visit.py | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-)