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
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:
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:
--
2.26.2
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:
© 2016 - 2025 Red Hat, Inc.