| Message ID | 20210119180242.1570753-11-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt1.5 | expand |
John Snow <jsnow@redhat.com> writes: > From: Markus Armbruster <armbru@redhat.com> > > QAPISchemaModularCVisitor attempts to encapsulate the way it splits > the module name space between user modules (name can't start with > './') and system modules (name is None or starts with './') by Is this still accurate? > providing separate ._add_user_module() and ._add_system_module(), > where the latter prepends './' to names other than None. > > Not worthwhile. Dumb down to a single ._add_module(). > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/commands.py | 2 +- > scripts/qapi/events.py | 2 +- > scripts/qapi/gen.py | 20 +++++++------------- > 3 files changed, 9 insertions(+), 15 deletions(-) > > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py > index fc5fe27c472..49111663394 100644 > --- a/scripts/qapi/commands.py > +++ b/scripts/qapi/commands.py > @@ -286,7 +286,7 @@ def _begin_user_module(self, name: str) -> None: > types=types)) > > def visit_end(self) -> None: > - self._add_system_module('./init', ' * QAPI Commands initialization') > + self._add_module('./init', ' * QAPI Commands initialization') > self._genh.add(mcgen(''' > #include "qapi/qmp/dispatch.h" > > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py > index 26faa829898..079c666ec69 100644 > --- a/scripts/qapi/events.py > +++ b/scripts/qapi/events.py > @@ -191,7 +191,7 @@ def _begin_user_module(self, name: str) -> None: > types=types)) > > def visit_end(self) -> None: > - self._add_system_module('./emit', ' * QAPI Events emission') > + self._add_module('./emit', ' * QAPI Events emission') > self._genc.preamble_add(mcgen(''' > #include "qemu/osdep.h" > #include "%(prefix)sqapi-emit-events.h" > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py > index 55acd7e080d..b5505685e6e 100644 > --- a/scripts/qapi/gen.py > +++ b/scripts/qapi/gen.py > @@ -272,22 +272,15 @@ def _module_filename(self, what: str, name: str) -> str: > self._module_basename(what, name)) > > def _add_module(self, name: str, blurb: str) -> None: > + if QAPISchemaModule.is_user_module(name): > + if self._main_module is None: > + self._main_module = name > basename = self._module_filename(self._what, name) > genc = QAPIGenC(basename + '.c', blurb, self._pydoc) > genh = QAPIGenH(basename + '.h', blurb, self._pydoc) > self._module[name] = (genc, genh) > self._genc, self._genh = self._module[name] > > - def _add_user_module(self, name: str, blurb: str) -> None: > - assert QAPISchemaModule.is_user_module(name) > - if self._main_module is None: > - self._main_module = name > - self._add_module(name, blurb) > - > - def _add_system_module(self, name: str, blurb: str) -> None: > - assert QAPISchemaModule.is_system_module(name) > - self._add_module(name, blurb) > - > def write(self, output_dir: str, opt_builtins: bool = False) -> None: > for name in self._module: > if QAPISchemaModule.is_builtin_module(name) and not opt_builtins: > @@ -303,9 +296,9 @@ def _begin_user_module(self, name: str) -> None: > pass > > def visit_module(self, module: QAPISchemaModule) -> None: > - if module.system_module: > + if module.builtin_module: Looks like you're fixing a slip-up in PATCH 06. If yes, squash. If no, I'm confused. > if self._builtin_blurb: > - self._add_system_module(module.name, self._builtin_blurb) > + self._add_module(module.name, self._builtin_blurb) > self._begin_builtin_module() > else: > # The built-in module has not been created. No code may > @@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) -> None: > self._genc = None > self._genh = None > else: > - self._add_user_module(module.name, self._user_blurb) > + assert module.user_module, "Unexpected system module" The string provides no value. > + self._add_module(module.name, self._user_blurb) > self._begin_user_module(module.name) > > def visit_include(self, name: str, info: QAPISourceInfo) -> None:
On 1/20/21 9:20 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> From: Markus Armbruster <armbru@redhat.com> >> >> QAPISchemaModularCVisitor attempts to encapsulate the way it splits >> the module name space between user modules (name can't start with >> './') and system modules (name is None or starts with './') by > > Is this still accurate? > Ah, not exactly; sorry I mangled your patches here. I had written my own version of what this patch used to be, but your patch went one step further, but the ordering got weird. >> providing separate ._add_user_module() and ._add_system_module(), >> where the latter prepends './' to names other than None. >> >> Not worthwhile. Dumb down to a single ._add_module(). >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/commands.py | 2 +- >> scripts/qapi/events.py | 2 +- >> scripts/qapi/gen.py | 20 +++++++------------- >> 3 files changed, 9 insertions(+), 15 deletions(-) >> >> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py >> index fc5fe27c472..49111663394 100644 >> --- a/scripts/qapi/commands.py >> +++ b/scripts/qapi/commands.py >> @@ -286,7 +286,7 @@ def _begin_user_module(self, name: str) -> None: >> types=types)) >> >> def visit_end(self) -> None: >> - self._add_system_module('./init', ' * QAPI Commands initialization') >> + self._add_module('./init', ' * QAPI Commands initialization') >> self._genh.add(mcgen(''' >> #include "qapi/qmp/dispatch.h" >> >> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py >> index 26faa829898..079c666ec69 100644 >> --- a/scripts/qapi/events.py >> +++ b/scripts/qapi/events.py >> @@ -191,7 +191,7 @@ def _begin_user_module(self, name: str) -> None: >> types=types)) >> >> def visit_end(self) -> None: >> - self._add_system_module('./emit', ' * QAPI Events emission') >> + self._add_module('./emit', ' * QAPI Events emission') >> self._genc.preamble_add(mcgen(''' >> #include "qemu/osdep.h" >> #include "%(prefix)sqapi-emit-events.h" >> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >> index 55acd7e080d..b5505685e6e 100644 >> --- a/scripts/qapi/gen.py >> +++ b/scripts/qapi/gen.py >> @@ -272,22 +272,15 @@ def _module_filename(self, what: str, name: str) -> str: >> self._module_basename(what, name)) >> >> def _add_module(self, name: str, blurb: str) -> None: >> + if QAPISchemaModule.is_user_module(name): >> + if self._main_module is None: >> + self._main_module = name >> basename = self._module_filename(self._what, name) >> genc = QAPIGenC(basename + '.c', blurb, self._pydoc) >> genh = QAPIGenH(basename + '.h', blurb, self._pydoc) >> self._module[name] = (genc, genh) >> self._genc, self._genh = self._module[name] >> >> - def _add_user_module(self, name: str, blurb: str) -> None: >> - assert QAPISchemaModule.is_user_module(name) >> - if self._main_module is None: >> - self._main_module = name >> - self._add_module(name, blurb) >> - >> - def _add_system_module(self, name: str, blurb: str) -> None: >> - assert QAPISchemaModule.is_system_module(name) >> - self._add_module(name, blurb) >> - >> def write(self, output_dir: str, opt_builtins: bool = False) -> None: >> for name in self._module: >> if QAPISchemaModule.is_builtin_module(name) and not opt_builtins: >> @@ -303,9 +296,9 @@ def _begin_user_module(self, name: str) -> None: >> pass >> >> def visit_module(self, module: QAPISchemaModule) -> None: >> - if module.system_module: >> + if module.builtin_module: > > Looks like you're fixing a slip-up in PATCH 06. If yes, squash. If no, > I'm confused. > Or patch 7, actually -- where we clarify that we are checking for the built-in module and not any old system module. >> if self._builtin_blurb: >> - self._add_system_module(module.name, self._builtin_blurb) >> + self._add_module(module.name, self._builtin_blurb) >> self._begin_builtin_module() >> else: >> # The built-in module has not been created. No code may >> @@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) -> None: >> self._genc = None >> self._genh = None >> else: >> - self._add_user_module(module.name, self._user_blurb) >> + assert module.user_module, "Unexpected system module" > > The string provides no value. > That's just, like, your opinion, man. It has value to me. Compare: ``` #!/usr/bin/env python3 def main(): assert False if __name__ == '__main__': main() ``` ``` # ./foo.py Traceback (most recent call last): File "./foo.py", line 7, in <module> main() File "./foo.py", line 4, in main assert False AssertionError ``` With: ``` #!/usr/bin/env python3 def main(): assert False, "Unexpected system module" if __name__ == '__main__': main() ``` ``` # ./foo.py Traceback (most recent call last): File "./foo.py", line 7, in <module> main() File "./foo.py", line 4, in main assert False, "Unexpected system module" AssertionError: Unexpected system module ``` I like the extra string for giving some semantic context as to specifically what broke (We don't expect to see system modules here) instead of just a stack trace. >> + self._add_module(module.name, self._user_blurb) >> self._begin_user_module(module.name) >> >> def visit_include(self, name: str, info: QAPISourceInfo) -> None:
John Snow <jsnow@redhat.com> writes: > On 1/20/21 9:20 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: [...] >>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >>> index 55acd7e080d..b5505685e6e 100644 >>> --- a/scripts/qapi/gen.py >>> +++ b/scripts/qapi/gen.py [...] >>> @@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) -> None: >>> self._genc = None >>> self._genh = None >>> else: >>> - self._add_user_module(module.name, self._user_blurb) >>> + assert module.user_module, "Unexpected system module" >> The string provides no value. >> > > That's just, like, your opinion, man. It has value to me. > > > Compare: > > ``` > #!/usr/bin/env python3 > > def main(): > assert False > > if __name__ == '__main__': > main() > ``` > > ``` > # ./foo.py > > Traceback (most recent call last): > File "./foo.py", line 7, in <module> > main() > File "./foo.py", line 4, in main > assert False > AssertionError > ``` > > With: > > > ``` > #!/usr/bin/env python3 > > def main(): > assert False, "Unexpected system module" > > if __name__ == '__main__': > main() > ``` > > ``` > # ./foo.py > > Traceback (most recent call last): > File "./foo.py", line 7, in <module> > main() > File "./foo.py", line 4, in main > assert False, "Unexpected system module" > AssertionError: Unexpected system module > ``` > > I like the extra string for giving some semantic context as to > specifically what broke (We don't expect to see system modules here) > instead of just a stack trace. Your test uses assert with an argument that tells us nothing. But the assert we're arguing about has a nice, expressive argument. The string improves the report from File "/work/armbru/qemu/scripts/qapi/gen.py", line 325, in visit_module assert module.user_module AssertionError to File "/work/armbru/qemu/scripts/qapi/gen.py", line 325, in visit_module assert module.user_module, "Unexpected system module" AssertionError: Unexpected system module Even if you value the difference, I very much doubt it justifies the clutter. Also, slippery slope towards pigs wearing way too much lipstick. Tested with diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index f3f4d2b011..bbfb30dc5a 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -321,6 +321,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): # be generated. self._current_module = None else: + module.name = "./HACK" assert module.user_module, "Unexpected system module" self._add_module(module.name, self._user_blurb) self._begin_user_module(module.name) > >>> + self._add_module(module.name, self._user_blurb) >>> self._begin_user_module(module.name) >>> def visit_include(self, name: str, info: QAPISourceInfo) -> >>> None:
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index fc5fe27c472..49111663394 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -286,7 +286,7 @@ def _begin_user_module(self, name: str) -> None: types=types)) def visit_end(self) -> None: - self._add_system_module('./init', ' * QAPI Commands initialization') + self._add_module('./init', ' * QAPI Commands initialization') self._genh.add(mcgen(''' #include "qapi/qmp/dispatch.h" diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index 26faa829898..079c666ec69 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -191,7 +191,7 @@ def _begin_user_module(self, name: str) -> None: types=types)) def visit_end(self) -> None: - self._add_system_module('./emit', ' * QAPI Events emission') + self._add_module('./emit', ' * QAPI Events emission') self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" #include "%(prefix)sqapi-emit-events.h" diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 55acd7e080d..b5505685e6e 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -272,22 +272,15 @@ def _module_filename(self, what: str, name: str) -> str: self._module_basename(what, name)) def _add_module(self, name: str, blurb: str) -> None: + if QAPISchemaModule.is_user_module(name): + if self._main_module is None: + self._main_module = name basename = self._module_filename(self._what, name) genc = QAPIGenC(basename + '.c', blurb, self._pydoc) genh = QAPIGenH(basename + '.h', blurb, self._pydoc) self._module[name] = (genc, genh) self._genc, self._genh = self._module[name] - def _add_user_module(self, name: str, blurb: str) -> None: - assert QAPISchemaModule.is_user_module(name) - if self._main_module is None: - self._main_module = name - self._add_module(name, blurb) - - def _add_system_module(self, name: str, blurb: str) -> None: - assert QAPISchemaModule.is_system_module(name) - self._add_module(name, blurb) - def write(self, output_dir: str, opt_builtins: bool = False) -> None: for name in self._module: if QAPISchemaModule.is_builtin_module(name) and not opt_builtins: @@ -303,9 +296,9 @@ def _begin_user_module(self, name: str) -> None: pass def visit_module(self, module: QAPISchemaModule) -> None: - if module.system_module: + if module.builtin_module: if self._builtin_blurb: - self._add_system_module(module.name, self._builtin_blurb) + self._add_module(module.name, self._builtin_blurb) self._begin_builtin_module() else: # The built-in module has not been created. No code may @@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) -> None: self._genc = None self._genh = None else: - self._add_user_module(module.name, self._user_blurb) + assert module.user_module, "Unexpected system module" + self._add_module(module.name, self._user_blurb) self._begin_user_module(module.name) def visit_include(self, name: str, info: QAPISourceInfo) -> None: