| Message ID | 55B00970.2060206@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/22/2015 03:21 PM, Eric Blake wrote: > 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. > >> > ...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). By the way, I'm perfectly fine if the initial conversion to the visitor keeps .c output separate into two groups to make review easier, then a later patch simplifies things back to a single sorting over the whole file (and undoing the grouping after conversion to visitor will be easier than figuring out how to interleave the builtin array visitors into the right sorted order prior to use of the visitor). I like the final result of everything sorted, it's just that I think it is easier to validate the final result when it is broken into stages with no one step doing too much, even if some of the intermediate stages do more work than needed and get undone by the final stage. Or put another way, your next spin of the series could include your original 26 + my hack 003 squashed together as one patch (conversion to visitor with minimized generated diff noise), followed by a revert of my hack 003 as the next (no need to group builtins together in the long run). [and for the record, I mention this because that's the approach I took in my earlier cleanups: commit fd41dd4e included a hack to rename 'type' to 'struct' in the generator, so that commit 895a2a80 could be quite mindless without touching the generator, and then commit 3e391d35 undid my hack. When I originally wrote the conversion in an earlier version of the series, I did not have that hack in place and the resulting single patch was too big; breaking it up made my submission and the review easier.]
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!
On 07/27/2015 10:09 AM, Markus Armbruster wrote: > >> 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. At this point, I'd be happy with just adding a script or other high-level instructions in the commit message that says how to divide a generated file into pieces (pull out all *List types into one piece, all typedefs into another, etc) and which can be done both pre- and post-patch. With pieces in hand, if you can easily compare that each pair is minimally different, then you have a nice reassurance that the difference in the overall file is due merely to differences in how the pieces are interleaved, and not to added or unintentionally dropped material. As you pointed out, there comes a point of diminishing returns in trying to clean up code that will just be discarded.
Eric Blake <eblake@redhat.com> writes: > On 07/27/2015 10:09 AM, Markus Armbruster wrote: > >> >>> 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. > > At this point, I'd be happy with just adding a script or other > high-level instructions in the commit message that says how to divide a > generated file into pieces (pull out all *List types into one piece, all > typedefs into another, etc) and which can be done both pre- and > post-patch. With pieces in hand, if you can easily compare that each > pair is minimally different, then you have a nice reassurance that the > difference in the overall file is due merely to differences in how the > pieces are interleaved, and not to added or unintentionally dropped > material. As you pointed out, there comes a point of diminishing > returns in trying to clean up code that will just be discarded. I'll give it a try.
From 348f3d3d3a35afe7dca4b695972677ce03da91ca Mon Sep 17 00:00:00 2001 From: Eric Blake <eblake@redhat.com> Date: Wed, 22 Jul 2015 14:58:25 -0600 Subject: [PATCH 4/4] qapi: squash to visitor Prior to conversion to a visitor, all code related to lists of builtins were lumped together. Doing so in the visitor code reduces the churn in comparing the generated files, making it easier to prove the conversion to the visitor is correct. --- scripts/qapi-types.py | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index d6185c6..0817628 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -240,27 +240,26 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self.defn = None self.fwdecl = None self.fwdefn = None - self.btin = None + self.btindecl = None + self.btindefn = None def visit_begin(self): self.decl = '' self.defn = '' self.fwdecl = '' self.fwdefn = '' - self.btin = guardstart('QAPI_TYPES_BUILTIN') + self.btindecl = guardstart('QAPI_TYPES_BUILTIN') + self.btindefn = '' 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 - # 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). + self.btindecl += guardend('QAPI_TYPES_BUILTIN') + self.decl = self.fwdecl + self.btindecl + self.decl + self.fwdecl = None + self.btindecl = None + self.defn = self.fwdefn + self.btindefn + self.defn + self.fwdefn = None + self.btindefn = None def _gen_type_cleanup(self, name): self.decl += generate_type_cleanup_decl(name) self.defn += generate_type_cleanup(name) @@ -269,11 +268,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): 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) + self.btindecl += gen_fwd_object_or_array(name) + self.btindecl += gen_array(name, element_type) + self.btindecl += generate_type_cleanup_decl(name) if do_builtins: - self.defn += generate_type_cleanup(name) + self.btindefn += generate_type_cleanup(name) else: self.fwdecl += gen_fwd_object_or_array(name) self.decl += gen_array(name, element_type) -- 2.4.3