| Message ID | 1435782155-31412-26-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/01/2015 02:22 PM, Markus Armbruster wrote: > Order of expressions doesn't matter. QAPISchema().get_exprs() returns > expressions in the order they're parsed. > > I'm going to refactor this code. To check refactorings, I want to > diff the generated code, and for that I need to preserve its order. > > Since I don't feel like preserving parse order, I'm changing > get_expr() to return the expressions sorted by name. This order will > be trivial to preserve. It also makes the generated code slightly > easier to navigate. Huge change to the generated files, but that's to be expected. Diffstat shows it is not a straight 1:1 reshuffle: qapi-event.c | 890 +-- qapi-event.h | 190 qapi-types.c | 1996 +++---- qapi-types.h | 5420 ++++++++++---------- qapi-visit.c | 9088 +++++++++++++++++----------------- qapi-visit.h | 746 +- qga/qapi-generated/qga-qapi-types.c | 148 qga/qapi-generated/qga-qapi-types.h | 340 - qga/qapi-generated/qga-qapi-visit.c | 446 - qga/qapi-generated/qga-qapi-visit.h | 54 qga/qapi-generated/qga-qmp-commands.h | 38 qga/qapi-generated/qga-qmp-marshal.c | 864 +-- qmp-commands.h | 376 - 13 files changed, 10308 insertions(+), 10288 deletions(-) but that's probably because you now emit forward declarations for things that occur alphabetically after their first use (since 8/47 in this series touched forward declarations), whereas before they happened to occurr in topological order from the parse. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi.py | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index cac7ab5..20ffdaf 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -1017,7 +1017,14 @@ class QAPISchema(object): > self.check() > > def get_exprs(self): > - return [expr_elem['expr'] for expr_elem in self.exprs] > + def expr_name(expr): > + name = expr.get('enum') or expr.get('union') \ > + or expr.get('alternate') or expr.get('struct') \ > + or expr.get('command') or expr.get('event') > + assert name > + return name When I was working on this file earlier, I half toyed with the idea of adding expr_elem['name'] holding the entity name, and expr_elem['meta'] holding what meta-type the entity represents; it might have made some of our later 6-way switches simpler. But with your hierarchy of actual objects, I'm not sure my idea helps any more. > + return sorted([expr_elem['expr'] for expr_elem in self.exprs], > + key=expr_name) Python has some nice compact syntactical gems - I'd hate the amount of boilerplate required to write this same filter using straight C code :) Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 07/01/2015 02:22 PM, Markus Armbruster wrote: >> Order of expressions doesn't matter. QAPISchema().get_exprs() returns >> expressions in the order they're parsed. >> >> I'm going to refactor this code. To check refactorings, I want to >> diff the generated code, and for that I need to preserve its order. >> >> Since I don't feel like preserving parse order, I'm changing >> get_expr() to return the expressions sorted by name. This order will >> be trivial to preserve. It also makes the generated code slightly >> easier to navigate. > > Huge change to the generated files, but that's to be expected. Diffstat > shows it is not a straight 1:1 reshuffle: > > qapi-event.c | 890 +-- > qapi-event.h | 190 > qapi-types.c | 1996 +++---- > qapi-types.h | 5420 ++++++++++---------- > qapi-visit.c | 9088 +++++++++++++++++----------------- > qapi-visit.h | 746 +- > qga/qapi-generated/qga-qapi-types.c | 148 > qga/qapi-generated/qga-qapi-types.h | 340 - > qga/qapi-generated/qga-qapi-visit.c | 446 - > qga/qapi-generated/qga-qapi-visit.h | 54 > qga/qapi-generated/qga-qmp-commands.h | 38 > qga/qapi-generated/qga-qmp-marshal.c | 864 +-- > qmp-commands.h | 376 - > 13 files changed, 10308 insertions(+), 10288 deletions(-) > > but that's probably because you now emit forward declarations for things > that occur alphabetically after their first use (since 8/47 in this > series touched forward declarations), whereas before they happened to > occurr in topological order from the parse. Here's my cheap trick for sanity checking this commit: compare before and after *sorted*. Results: * qapi-event.h - Members of enum QAPIEvent reordered, values changed accordingly (but they shouldn't matter) * qapi-visit.c - A bunch of new forward declarations * test-qapi-visit.c - One forward declaration gone. >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> scripts/qapi.py | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/scripts/qapi.py b/scripts/qapi.py >> index cac7ab5..20ffdaf 100644 >> --- a/scripts/qapi.py >> +++ b/scripts/qapi.py >> @@ -1017,7 +1017,14 @@ class QAPISchema(object): >> self.check() >> >> def get_exprs(self): >> - return [expr_elem['expr'] for expr_elem in self.exprs] >> + def expr_name(expr): >> + name = expr.get('enum') or expr.get('union') \ >> + or expr.get('alternate') or expr.get('struct') \ >> + or expr.get('command') or expr.get('event') >> + assert name >> + return name > > When I was working on this file earlier, I half toyed with the idea of > adding expr_elem['name'] holding the entity name, and expr_elem['meta'] > holding what meta-type the entity represents; it might have made some of > our later 6-way switches simpler. But with your hierarchy of actual > objects, I'm not sure my idea helps any more. The more work we do with the new internal representation instead of the syntax tree, the less it'll help. >> + return sorted([expr_elem['expr'] for expr_elem in self.exprs], >> + key=expr_name) > > Python has some nice compact syntactical gems - I'd hate the amount of > boilerplate required to write this same filter using straight C code :) > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
diff the generated code, and for that I need to preserve its order. Since I don't feel like preserving parse order, I'm changing get_expr() to return the expressions sorted by name. This order will be trivial to preserve. It also makes the generated code slightly easier to navigate. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index cac7ab5..20ffdaf 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1017,7 +1017,14 @@ class QAPISchema(object): self.check() def get_exprs(self): - return [expr_elem['expr'] for expr_elem in self.exprs] + def expr_name(expr): + name = expr.get('enum') or expr.get('union') \ + or expr.get('alternate') or expr.get('struct') \ + or expr.get('command') or expr.get('event') + assert name + return name + return sorted([expr_elem['expr'] for expr_elem in self.exprs], + key=expr_name) def _def_entity(self, ent): assert ent.name not in self.entity_dict