* [PATCH v3 01/17] qapi/commands: assert arg_type is not None
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 02/17] qapi/events: fix visit_event typing John Snow
` (15 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
When boxed is True, expr.py asserts that we must have
arguments. Ultimately, this should mean that if boxed is True that
arg_type should be defined. Mypy cannot infer this, and does not support
'stateful' type inference, e.g.:
```
if x:
assert y is not None
...
if x:
y.etc()
```
does not work, because mypy does not statefully remember the conditional
assertion in the second block. Help mypy out by creating a new local
that it can track more easily.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/commands.py | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 50978090b44..71744f48a35 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -126,6 +126,9 @@ def gen_marshal(name: str,
boxed: bool,
ret_type: Optional[QAPISchemaType]) -> str:
have_args = boxed or (arg_type and not arg_type.is_empty())
+ if have_args:
+ assert arg_type is not None
+ arg_type_c_name = arg_type.c_name()
ret = mcgen('''
@@ -147,7 +150,7 @@ def gen_marshal(name: str,
ret += mcgen('''
%(c_name)s arg = {0};
''',
- c_name=arg_type.c_name())
+ c_name=arg_type_c_name)
ret += mcgen('''
@@ -163,7 +166,7 @@ def gen_marshal(name: str,
ok = visit_check_struct(v, errp);
}
''',
- c_arg_type=arg_type.c_name())
+ c_arg_type=arg_type_c_name)
else:
ret += mcgen('''
ok = visit_check_struct(v, errp);
@@ -193,7 +196,7 @@ def gen_marshal(name: str,
ret += mcgen('''
visit_type_%(c_arg_type)s_members(v, &arg, NULL);
''',
- c_arg_type=arg_type.c_name())
+ c_arg_type=arg_type_c_name)
ret += mcgen('''
visit_end_struct(v, NULL);
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 02/17] qapi/events: fix visit_event typing
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
2021-01-19 18:02 ` [PATCH v3 01/17] qapi/commands: assert arg_type is not None John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 03/17] qapi/main: handle theoretical None-return from re.match() John Snow
` (14 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
Actually, the arg_type can indeed be Optional.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/events.py | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 599f3d1f564..9851653b9d1 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -12,7 +12,7 @@
See the COPYING file in the top-level directory.
"""
-from typing import List
+from typing import List, Optional
from .common import c_enum_const, c_name, mcgen
from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
@@ -27,7 +27,7 @@
def build_event_send_proto(name: str,
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> str:
return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
'c_name': c_name(name.lower()),
@@ -35,7 +35,7 @@ def build_event_send_proto(name: str,
def gen_event_send_decl(name: str,
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> str:
return mcgen('''
@@ -78,7 +78,7 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str:
def gen_event_send(name: str,
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool,
event_enum_name: str,
event_emit: str) -> str:
@@ -99,6 +99,7 @@ def gen_event_send(name: str,
proto=build_event_send_proto(name, arg_type, boxed))
if have_args:
+ assert arg_type is not None
ret += mcgen('''
QObject *obj;
Visitor *v;
@@ -114,6 +115,7 @@ def gen_event_send(name: str,
name=name)
if have_args:
+ assert arg_type is not None
ret += mcgen('''
v = qobject_output_visitor_new(&obj);
''')
@@ -214,7 +216,7 @@ def visit_event(self,
info: QAPISourceInfo,
ifcond: List[str],
features: List[QAPISchemaFeature],
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> None:
with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_event_send_decl(name, arg_type, boxed))
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 03/17] qapi/main: handle theoretical None-return from re.match()
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
2021-01-19 18:02 ` [PATCH v3 01/17] qapi/commands: assert arg_type is not None John Snow
2021-01-19 18:02 ` [PATCH v3 02/17] qapi/events: fix visit_event typing John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 04/17] qapi/gen: inline _wrap_ifcond into end_if() John Snow
` (13 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
Mypy cannot understand that this match can never be None, so help it
along.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/main.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 42517210b80..703e7ed1ed5 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -23,6 +23,8 @@
def invalid_prefix_char(prefix: str) -> Optional[str]:
match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+ # match cannot be None, but mypy cannot infer that.
+ assert match is not None
if match.end() != len(prefix):
return prefix[match.end()]
return None
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 04/17] qapi/gen: inline _wrap_ifcond into end_if()
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (2 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 03/17] qapi/main: handle theoretical None-return from re.match() John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str John Snow
` (12 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
We assert _start_if is not None in end_if, but that's opaque to mypy.
By inlining _wrap_ifcond, that constraint becomes provable to mypy.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b40f18eee3c..3d81b90ab71 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -130,15 +130,12 @@ def start_if(self, ifcond: List[str]) -> None:
self._start_if = (ifcond, self._body, self._preamble)
def end_if(self) -> None:
- assert self._start_if
- self._wrap_ifcond()
- self._start_if = None
-
- def _wrap_ifcond(self) -> None:
+ assert self._start_if is not None
self._body = _wrap_ifcond(self._start_if[0],
self._start_if[1], self._body)
self._preamble = _wrap_ifcond(self._start_if[0],
self._start_if[2], self._preamble)
+ self._start_if = None
def get_content(self) -> str:
assert self._start_if is None
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (3 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 04/17] qapi/gen: inline _wrap_ifcond into end_if() John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-20 12:07 ` Markus Armbruster
2021-01-19 18:02 ` [PATCH v3 06/17] qapi: centralize is_[user|system|builtin]_module methods John Snow
` (11 subsequent siblings)
16 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
Modify visit_module to pass the module itself instead of just its
name. This allows for future patches to centralize some
module-interrogation behavior within the QAPISchemaModule class itself,
cutting down on duplication between gen.py and schema.py.
Signed-off-by: John Snow <jsnow@redhat.com>
---
docs/sphinx/qapidoc.py | 8 ++++----
scripts/qapi/gen.py | 16 ++++++++++------
scripts/qapi/schema.py | 4 ++--
tests/qapi-schema/test-qapi.py | 4 ++--
4 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index e03abcbb959..f754f675d66 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
self._env = env
self._qapidir = qapidir
- def visit_module(self, name):
- if name is not None:
- qapifile = self._qapidir + '/' + name
+ def visit_module(self, module):
+ if module.name:
+ qapifile = self._qapidir + '/' + module.name
self._env.note_dependency(os.path.abspath(qapifile))
- super().visit_module(name)
+ super().visit_module(module)
class QAPIDocDirective(Directive):
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 3d81b90ab71..e73d3d61aac 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -31,7 +31,11 @@
guardstart,
mcgen,
)
-from .schema import QAPISchemaObjectType, QAPISchemaVisitor
+from .schema import (
+ QAPISchemaModule,
+ QAPISchemaObjectType,
+ QAPISchemaVisitor,
+)
from .source import QAPISourceInfo
@@ -304,19 +308,19 @@ def _begin_system_module(self, name: None) -> None:
def _begin_user_module(self, name: str) -> None:
pass
- def visit_module(self, name: Optional[str]) -> None:
- if name is None:
+ def visit_module(self, module: QAPISchemaModule) -> None:
+ if module.name is None:
if self._builtin_blurb:
self._add_system_module(None, self._builtin_blurb)
- self._begin_system_module(name)
+ self._begin_system_module(module.name)
else:
# The built-in module has not been created. No code may
# be generated.
self._genc = None
self._genh = None
else:
- self._add_user_module(name, self._user_blurb)
- self._begin_user_module(name)
+ self._add_user_module(module.name, self._user_blurb)
+ self._begin_user_module(module.name)
def visit_include(self, name: str, info: QAPISourceInfo) -> None:
relname = os.path.relpath(self._module_filename(self._what, name),
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4..69ba722c084 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -97,7 +97,7 @@ def visit_begin(self, schema):
def visit_end(self):
pass
- def visit_module(self, name):
+ def visit_module(self, module):
pass
def visit_needed(self, entity):
@@ -145,7 +145,7 @@ def add_entity(self, ent):
self._entity_list.append(ent)
def visit(self, visitor):
- visitor.visit_module(self.name)
+ visitor.visit_module(self)
for entity in self._entity_list:
if visitor.visit_needed(entity):
entity.visit(visitor)
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index e8db9d09d91..bec1ebff3db 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -24,8 +24,8 @@
class QAPISchemaTestVisitor(QAPISchemaVisitor):
- def visit_module(self, name):
- print('module %s' % name)
+ def visit_module(self, module):
+ print('module %s' % module.name)
def visit_include(self, name, info):
print('include %s' % name)
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str
2021-01-19 18:02 ` [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str John Snow
@ 2021-01-20 12:07 ` Markus Armbruster
2021-01-20 15:51 ` John Snow
2021-01-20 16:02 ` Eric Blake
0 siblings, 2 replies; 29+ messages in thread
From: Markus Armbruster @ 2021-01-20 12:07 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Modify visit_module to pass the module itself instead of just its
> name. This allows for future patches to centralize some
> module-interrogation behavior within the QAPISchemaModule class itself,
> cutting down on duplication between gen.py and schema.py.
We've been tempted to make similar changes before (don't worry, I'm not
building a case for "no" here).
When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
2015), I aimed for a loose coupling of backends and the internal
representation. Instead of
def visit_foo(self, foo):
pass
where @foo is a QAPISchemaFooBar, I wrote
def visit_foo_bar(self, name, info, [curated attributes of @foo]):
pass
In theory, this is nice: the information exposed to the backends is
obvious, and the backends can't accidentally mutate @foo.
In practice, it kind of failed right then and there:
def visit_object_type(self, name, info, base, members, variants):
pass
We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
to pass member information as List[QAPISchemaObjectTypeMember].
Morever, passing "curated atttibutes" has led to visit_commands() taking
a dozen arguments. Meh.
This had made Eric and me wonder whether we should write off the
decoupling idea as misguided, and just pass the object instead of
"curated attributes", always. Thoughts?
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> docs/sphinx/qapidoc.py | 8 ++++----
> scripts/qapi/gen.py | 16 ++++++++++------
> scripts/qapi/schema.py | 4 ++--
> tests/qapi-schema/test-qapi.py | 4 ++--
> 4 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index e03abcbb959..f754f675d66 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
> self._env = env
> self._qapidir = qapidir
>
> - def visit_module(self, name):
> - if name is not None:
> - qapifile = self._qapidir + '/' + name
> + def visit_module(self, module):
> + if module.name:
Replacing the "is not None" test by (implicit) "is thruthy" changes
behavior for the empty string. Intentional?
I've had the "pleasure" of debugging empty strings getting interpreted
like None where they should be interpreted like any other string.
> + qapifile = self._qapidir + '/' + module.name
> self._env.note_dependency(os.path.abspath(qapifile))
> - super().visit_module(name)
> + super().visit_module(module)
>
>
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str
2021-01-20 12:07 ` Markus Armbruster
@ 2021-01-20 15:51 ` John Snow
2021-01-21 7:22 ` Markus Armbruster
2021-01-20 16:02 ` Eric Blake
1 sibling, 1 reply; 29+ messages in thread
From: John Snow @ 2021-01-20 15:51 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
On 1/20/21 7:07 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Modify visit_module to pass the module itself instead of just its
>> name. This allows for future patches to centralize some
>> module-interrogation behavior within the QAPISchemaModule class itself,
>> cutting down on duplication between gen.py and schema.py.
>
> We've been tempted to make similar changes before (don't worry, I'm not
> building a case for "no" here).
>
It's fine: you'll probably notice later I don't go the full distance and
rely on both object and class methods anyway, so this isn't strictly
needed right now.
(It was not possible to go the full distance without heavier, more
invasive changes, so...)
> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
> 2015), I aimed for a loose coupling of backends and the internal
> representation. Instead of
>
> def visit_foo(self, foo):
> pass
>
> where @foo is a QAPISchemaFooBar, I wrote
>
> def visit_foo_bar(self, name, info, [curated attributes of @foo]):
> pass
>
> In theory, this is nice: the information exposed to the backends is
> obvious, and the backends can't accidentally mutate @foo.
>
> In practice, it kind of failed right then and there:
>
> def visit_object_type(self, name, info, base, members, variants):
> pass
>
> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
> to pass member information as List[QAPISchemaObjectTypeMember].
>
> Morever, passing "curated atttibutes" has led to visit_commands() taking
> a dozen arguments. Meh.
>
> This had made Eric and me wonder whether we should write off the
> decoupling idea as misguided, and just pass the object instead of
> "curated attributes", always. Thoughts?
>
I'm not sure. Just taking the object would avoid a lot of duplicated
interface typing, and type hints would allow editors to know what fields
are available.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> docs/sphinx/qapidoc.py | 8 ++++----
>> scripts/qapi/gen.py | 16 ++++++++++------
>> scripts/qapi/schema.py | 4 ++--
>> tests/qapi-schema/test-qapi.py | 4 ++--
>> 4 files changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> index e03abcbb959..f754f675d66 100644
>> --- a/docs/sphinx/qapidoc.py
>> +++ b/docs/sphinx/qapidoc.py
>> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
>> self._env = env
>> self._qapidir = qapidir
>>
>> - def visit_module(self, name):
>> - if name is not None:
>> - qapifile = self._qapidir + '/' + name
>> + def visit_module(self, module):
>> + if module.name:
>
> Replacing the "is not None" test by (implicit) "is thruthy" changes
> behavior for the empty string. Intentional?
>
Instinctively it was intentional, consciously it wasn't. I was worried
about what "qapifile" would produce if the string happened to be empty.
> I've had the "pleasure" of debugging empty strings getting interpreted
> like None where they should be interpreted like any other string.
>
assert module.name, then?
>> + qapifile = self._qapidir + '/' + module.name
>> self._env.note_dependency(os.path.abspath(qapifile))
>> - super().visit_module(name)
>> + super().visit_module(module)
>>
>>
> [...]
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str
2021-01-20 15:51 ` John Snow
@ 2021-01-21 7:22 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2021-01-21 7:22 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 1/20/21 7:07 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
[...]
>>> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>>> index e03abcbb959..f754f675d66 100644
>>> --- a/docs/sphinx/qapidoc.py
>>> +++ b/docs/sphinx/qapidoc.py
>>> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
>>> self._env = env
>>> self._qapidir = qapidir
>>> - def visit_module(self, name):
>>> - if name is not None:
>>> - qapifile = self._qapidir + '/' + name
>>> + def visit_module(self, module):
>>> + if module.name:
>> Replacing the "is not None" test by (implicit) "is thruthy" changes
>> behavior for the empty string. Intentional?
>>
>
> Instinctively it was intentional, consciously it wasn't. I was worried
> about what "qapifile" would produce if the string happened to be
> empty.
It would produce a dependency on the directory.
>> I've had the "pleasure" of debugging empty strings getting interpreted
>> like None where they should be interpreted like any other string.
>>
>
> assert module.name, then?
Let's avoid changing behavior in a refactoring patch. If you want an
assertion to ease your worry, separate patch.
>>> + qapifile = self._qapidir + '/' + module.name
>>> self._env.note_dependency(os.path.abspath(qapifile))
>>> - super().visit_module(name)
>>> + super().visit_module(module)
>>>
>> [...]
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str
2021-01-20 12:07 ` Markus Armbruster
2021-01-20 15:51 ` John Snow
@ 2021-01-20 16:02 ` Eric Blake
2021-01-20 16:16 ` John Snow
1 sibling, 1 reply; 29+ messages in thread
From: Eric Blake @ 2021-01-20 16:02 UTC (permalink / raw)
To: Markus Armbruster, John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
On 1/20/21 6:07 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Modify visit_module to pass the module itself instead of just its
>> name. This allows for future patches to centralize some
>> module-interrogation behavior within the QAPISchemaModule class itself,
>> cutting down on duplication between gen.py and schema.py.
>
> We've been tempted to make similar changes before (don't worry, I'm not
> building a case for "no" here).
>
> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
> 2015), I aimed for a loose coupling of backends and the internal
> representation. Instead of
>
> def visit_foo(self, foo):
> pass
>
> where @foo is a QAPISchemaFooBar, I wrote
>
> def visit_foo_bar(self, name, info, [curated attributes of @foo]):
> pass
>
> In theory, this is nice: the information exposed to the backends is
> obvious, and the backends can't accidentally mutate @foo.
>
> In practice, it kind of failed right then and there:
>
> def visit_object_type(self, name, info, base, members, variants):
> pass
>
> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
> to pass member information as List[QAPISchemaObjectTypeMember].
>
> Morever, passing "curated atttibutes" has led to visit_commands() taking
> a dozen arguments. Meh.
>
> This had made Eric and me wonder whether we should write off the
> decoupling idea as misguided, and just pass the object instead of
> "curated attributes", always. Thoughts?
I'm open to the idea of passing just the larger object instead of the
curated list of relevant attributes. It's a bit more coupling, but I
don't see any of our qapi code being reused outside its current scope
where the extra coupling will bite us. But I'm not volunteering for the
refactoring work, because I'm not an expert on python typing hints. If
consolidating parameters into the larger object makes for fewer
parameters and easier typing hints, I'm assuming the work can be done as
part of static typing; but if leaving things as they currently are is
manageable, that's also fine by me.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str
2021-01-20 16:02 ` Eric Blake
@ 2021-01-20 16:16 ` John Snow
2021-01-21 7:23 ` Markus Armbruster
0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2021-01-20 16:16 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
On 1/20/21 11:02 AM, Eric Blake wrote:
> On 1/20/21 6:07 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Modify visit_module to pass the module itself instead of just its
>>> name. This allows for future patches to centralize some
>>> module-interrogation behavior within the QAPISchemaModule class itself,
>>> cutting down on duplication between gen.py and schema.py.
>>
>> We've been tempted to make similar changes before (don't worry, I'm not
>> building a case for "no" here).
>>
>> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
>> 2015), I aimed for a loose coupling of backends and the internal
>> representation. Instead of
>>
>> def visit_foo(self, foo):
>> pass
>>
>> where @foo is a QAPISchemaFooBar, I wrote
>>
>> def visit_foo_bar(self, name, info, [curated attributes of @foo]):
>> pass
>>
>> In theory, this is nice: the information exposed to the backends is
>> obvious, and the backends can't accidentally mutate @foo.
>>
>> In practice, it kind of failed right then and there:
>>
>> def visit_object_type(self, name, info, base, members, variants):
>> pass
>>
>> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
>> to pass member information as List[QAPISchemaObjectTypeMember].
>>
>> Morever, passing "curated atttibutes" has led to visit_commands() taking
>> a dozen arguments. Meh.
>>
>> This had made Eric and me wonder whether we should write off the
>> decoupling idea as misguided, and just pass the object instead of
>> "curated attributes", always. Thoughts?
>
> I'm open to the idea of passing just the larger object instead of the
> curated list of relevant attributes. It's a bit more coupling, but I
> don't see any of our qapi code being reused outside its current scope
> where the extra coupling will bite us. But I'm not volunteering for the
> refactoring work, because I'm not an expert on python typing hints. If
> consolidating parameters into the larger object makes for fewer
> parameters and easier typing hints, I'm assuming the work can be done as
> part of static typing; but if leaving things as they currently are is
> manageable, that's also fine by me.
>
Yeah, it can definitely be left as-is for now. I've already gone through
all the effort of typing out all of the interfaces, so it's not really a
huge ordeal to just leave it as-is.
Passing the objects might be nicer for the future, though, as routing
new information or features will involve less churn. (And the signatures
will be a lot smaller.)
I suppose it does open us up to callers mutating the schema in the
visitors, but they could already do that for the reasons that Markus
points out. It's possible that the visitor dispatch could be modified to
make deep copies of schema objects, but that adds overhead.
I can just revert this change for now and leave the topic for another day.
--js
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str
2021-01-20 16:16 ` John Snow
@ 2021-01-21 7:23 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2021-01-21 7:23 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 1/20/21 11:02 AM, Eric Blake wrote:
>> On 1/20/21 6:07 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Modify visit_module to pass the module itself instead of just its
>>>> name. This allows for future patches to centralize some
>>>> module-interrogation behavior within the QAPISchemaModule class itself,
>>>> cutting down on duplication between gen.py and schema.py.
>>>
>>> We've been tempted to make similar changes before (don't worry, I'm not
>>> building a case for "no" here).
>>>
>>> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
>>> 2015), I aimed for a loose coupling of backends and the internal
>>> representation. Instead of
>>>
>>> def visit_foo(self, foo):
>>> pass
>>>
>>> where @foo is a QAPISchemaFooBar, I wrote
>>>
>>> def visit_foo_bar(self, name, info, [curated attributes of @foo]):
>>> pass
>>>
>>> In theory, this is nice: the information exposed to the backends is
>>> obvious, and the backends can't accidentally mutate @foo.
>>>
>>> In practice, it kind of failed right then and there:
>>>
>>> def visit_object_type(self, name, info, base, members, variants):
>>> pass
>>>
>>> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
>>> to pass member information as List[QAPISchemaObjectTypeMember].
>>>
>>> Morever, passing "curated atttibutes" has led to visit_commands() taking
>>> a dozen arguments. Meh.
>>>
>>> This had made Eric and me wonder whether we should write off the
>>> decoupling idea as misguided, and just pass the object instead of
>>> "curated attributes", always. Thoughts?
>> I'm open to the idea of passing just the larger object instead of
>> the
>> curated list of relevant attributes. It's a bit more coupling, but I
>> don't see any of our qapi code being reused outside its current scope
>> where the extra coupling will bite us. But I'm not volunteering for the
>> refactoring work, because I'm not an expert on python typing hints. If
>> consolidating parameters into the larger object makes for fewer
>> parameters and easier typing hints, I'm assuming the work can be done as
>> part of static typing; but if leaving things as they currently are is
>> manageable, that's also fine by me.
>>
>
> Yeah, it can definitely be left as-is for now. I've already gone
> through all the effort of typing out all of the interfaces, so it's
> not really a huge ordeal to just leave it as-is.
>
> Passing the objects might be nicer for the future, though, as routing
> new information or features will involve less churn. (And the
> signatures will be a lot smaller.)
>
> I suppose it does open us up to callers mutating the schema in the
> visitors, but they could already do that for the reasons that Markus
> points out. It's possible that the visitor dispatch could be modified
> to make deep copies of schema objects, but that adds overhead.
>
> I can just revert this change for now and leave the topic for another day.
Works for me. Thanks!
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 06/17] qapi: centralize is_[user|system|builtin]_module methods
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (4 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-20 13:56 ` Markus Armbruster
2021-01-19 18:02 ` [PATCH v3 07/17] qapi/gen: Replace ._begin_system_module() John Snow
` (10 subsequent siblings)
16 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
Define what a module is and define what kind of a module it is once and
for all, in one place.
Signed-off-by: John Snow <jsnow@redhat.com>
---
docs/sphinx/qapidoc.py | 2 +-
scripts/qapi/gen.py | 21 +++++++--------------
scripts/qapi/schema.py | 28 ++++++++++++++++++++++++++--
3 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index f754f675d66..08b9439de2b 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -464,7 +464,7 @@ def __init__(self, env, qapidir):
self._qapidir = qapidir
def visit_module(self, module):
- if module.name:
+ if module.user_module:
qapifile = self._qapidir + '/' + module.name
self._env.note_dependency(os.path.abspath(qapifile))
super().visit_module(module)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index e73d3d61aac..3b7eddc1c21 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -250,21 +250,14 @@ def __init__(self,
self._main_module: Optional[str] = None
@staticmethod
- def _is_user_module(name: Optional[str]) -> bool:
- return bool(name and not name.startswith('./'))
-
- @staticmethod
- def _is_builtin_module(name: Optional[str]) -> bool:
- return not name
-
- def _module_dirname(self, name: Optional[str]) -> str:
- if self._is_user_module(name):
+ def _module_dirname(name: Optional[str]) -> str:
+ if QAPISchemaModule.is_user_module(name):
return os.path.dirname(name)
return ''
def _module_basename(self, what: str, name: Optional[str]) -> str:
- ret = '' if self._is_builtin_module(name) else self._prefix
- if self._is_user_module(name):
+ ret = '' if QAPISchemaModule.is_builtin_module(name) else self._prefix
+ if QAPISchemaModule.is_user_module(name):
basename = os.path.basename(name)
ret += what
if name != self._main_module:
@@ -286,7 +279,7 @@ def _add_module(self, name: Optional[str], blurb: str) -> None:
self._genc, self._genh = self._module[name]
def _add_user_module(self, name: str, blurb: str) -> None:
- assert self._is_user_module(name)
+ assert QAPISchemaModule.is_user_module(name)
if self._main_module is None:
self._main_module = name
self._add_module(name, blurb)
@@ -296,7 +289,7 @@ def _add_system_module(self, name: Optional[str], blurb: str) -> None:
def write(self, output_dir: str, opt_builtins: bool = False) -> None:
for name in self._module:
- if self._is_builtin_module(name) and not opt_builtins:
+ if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
continue
(genc, genh) = self._module[name]
genc.write(output_dir)
@@ -309,7 +302,7 @@ def _begin_user_module(self, name: str) -> None:
pass
def visit_module(self, module: QAPISchemaModule) -> None:
- if module.name is None:
+ if module.system_module:
if self._builtin_blurb:
self._add_system_module(None, self._builtin_blurb)
self._begin_system_module(module.name)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 69ba722c084..44a9232c881 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -141,6 +141,30 @@ def __init__(self, name):
self.name = name
self._entity_list = []
+ @classmethod
+ def is_system_module(cls, name: Optional[str]) -> bool:
+ return name is None or name.startswith('./')
+
+ @classmethod
+ def is_user_module(cls, name: Optional[str]) -> bool:
+ return not cls.is_system_module(name)
+
+ @classmethod
+ def is_builtin_module(cls, name: str) -> bool:
+ return name == './builtin'
+
+ @property
+ def system_module(self) -> bool:
+ return self.is_system_module(self.name)
+
+ @property
+ def user_module(self) -> bool:
+ return self.is_user_module(self.name)
+
+ @property
+ def builtin_module(self) -> bool:
+ return self.is_builtin_module(self.name)
+
def add_entity(self, ent):
self._entity_list.append(ent)
@@ -871,8 +895,8 @@ def resolve_type(self, name, info, what):
return typ
def _module_name(self, fname):
- if fname is None:
- return None
+ if QAPISchemaModule.is_system_module(fname):
+ return fname
return os.path.relpath(fname, self._schema_dir)
def _make_module(self, fname):
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 06/17] qapi: centralize is_[user|system|builtin]_module methods
2021-01-19 18:02 ` [PATCH v3 06/17] qapi: centralize is_[user|system|builtin]_module methods John Snow
@ 2021-01-20 13:56 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2021-01-20 13:56 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Define what a module is and define what kind of a module it is once and
> for all, in one place.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> docs/sphinx/qapidoc.py | 2 +-
> scripts/qapi/gen.py | 21 +++++++--------------
> scripts/qapi/schema.py | 28 ++++++++++++++++++++++++++--
> 3 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index f754f675d66..08b9439de2b 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -464,7 +464,7 @@ def __init__(self, env, qapidir):
> self._qapidir = qapidir
>
> def visit_module(self, module):
> - if module.name:
> + if module.user_module:
> qapifile = self._qapidir + '/' + module.name
> self._env.note_dependency(os.path.abspath(qapifile))
> super().visit_module(module)
Cleaner:
qapifile = module.source_filename()
if qapifile:
self._env.note_dependency(os.path.abspath(qapifile))
Observation, not demand.
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index e73d3d61aac..3b7eddc1c21 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -250,21 +250,14 @@ def __init__(self,
> self._main_module: Optional[str] = None
>
> @staticmethod
> - def _is_user_module(name: Optional[str]) -> bool:
> - return bool(name and not name.startswith('./'))
> -
> - @staticmethod
> - def _is_builtin_module(name: Optional[str]) -> bool:
> - return not name
> -
> - def _module_dirname(self, name: Optional[str]) -> str:
> - if self._is_user_module(name):
> + def _module_dirname(name: Optional[str]) -> str:
> + if QAPISchemaModule.is_user_module(name):
> return os.path.dirname(name)
> return ''
No use of self left. @staticmethod?
Or maybe ...
>
> def _module_basename(self, what: str, name: Optional[str]) -> str:
> - ret = '' if self._is_builtin_module(name) else self._prefix
> - if self._is_user_module(name):
> + ret = '' if QAPISchemaModule.is_builtin_module(name) else self._prefix
> + if QAPISchemaModule.is_user_module(name):
> basename = os.path.basename(name)
> ret += what
> if name != self._main_module:
if name != self._main_module:
ret += '-' + os.path.splitext(basename)[0]
else:
name = name[2:] if name else 'builtin'
ret += re.sub(r'-', '-' + name + '-', what)
return ret
def _module_filename(self, what: str, name: Optional[str]) -> str:
return os.path.join(self._module_dirname(name),
self._module_basename(what, name))
... replace this by
return os.path.join(module.source_dirname(),
self._module_basename(what, name))
Requires plumbing @module from visit_module() to here.
Again, not a demand.
> @@ -286,7 +279,7 @@ def _add_module(self, name: Optional[str], blurb: str) -> None:
> self._genc, self._genh = self._module[name]
>
> def _add_user_module(self, name: str, blurb: str) -> None:
> - assert self._is_user_module(name)
> + assert QAPISchemaModule.is_user_module(name)
> if self._main_module is None:
> self._main_module = name
> self._add_module(name, blurb)
> @@ -296,7 +289,7 @@ def _add_system_module(self, name: Optional[str], blurb: str) -> None:
>
> def write(self, output_dir: str, opt_builtins: bool = False) -> None:
> for name in self._module:
> - if self._is_builtin_module(name) and not opt_builtins:
> + if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
> continue
> (genc, genh) = self._module[name]
> genc.write(output_dir)
> @@ -309,7 +302,7 @@ def _begin_user_module(self, name: str) -> None:
> pass
>
> def visit_module(self, module: QAPISchemaModule) -> None:
> - if module.name is None:
> + if module.system_module:
> if self._builtin_blurb:
> self._add_system_module(None, self._builtin_blurb)
> self._begin_system_module(module.name)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 69ba722c084..44a9232c881 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -141,6 +141,30 @@ def __init__(self, name):
> self.name = name
> self._entity_list = []
>
> + @classmethod
> + def is_system_module(cls, name: Optional[str]) -> bool:
> + return name is None or name.startswith('./')
> +
> + @classmethod
> + def is_user_module(cls, name: Optional[str]) -> bool:
> + return not cls.is_system_module(name)
> +
> + @classmethod
> + def is_builtin_module(cls, name: str) -> bool:
> + return name == './builtin'
The names suggest these are predicates on modules, but they're actually
predicates on module *names*.
@staticmethod would suffice. Actually, *functions* would. All the
"classiness" buys us here is having to write
QAPISchemaModule.is_FOO_module(name) instead of is_FOO_module(name), and
...
If you *want* classy, have a class QAPISchemaModuleName.
> +
> + @property
> + def system_module(self) -> bool:
> + return self.is_system_module(self.name)
> +
> + @property
> + def user_module(self) -> bool:
> + return self.is_user_module(self.name)
> +
> + @property
> + def builtin_module(self) -> bool:
> + return self.is_builtin_module(self.name)
... module.FOO_module instead of module.is_FOO_module.
Any particular reason for the @property decorator?
> +
> def add_entity(self, ent):
> self._entity_list.append(ent)
>
> @@ -871,8 +895,8 @@ def resolve_type(self, name, info, what):
> return typ
>
> def _module_name(self, fname):
> - if fname is None:
> - return None
> + if QAPISchemaModule.is_system_module(fname):
> + return fname
> return os.path.relpath(fname, self._schema_dir)
>
> def _make_module(self, fname):
name = self._module_name(fname)
if name not in self._module_dict:
self._module_dict[name] = QAPISchemaModule(name)
return self._module_dict[name]
I got one more issue.
Before the patch:
* QAPISchemaModule deals with the user's modules and a single, special
module for built-ins. The former's name is the schema module filename
relative to the main schema's directory, the latter's name is None.
* QAPISchemaModularCVisitor supports both kinds, and adds a third kind:
system modules. Their name begins with './', which cannot clash.
These guys are used for generated code that doesn't fit anywhere else.
Your patch drags system modules into QAPISchemaModule. Somewhat
awkward. Could use comments (but then, much of scripts/qapi/ could).
Subtyping QAPISchemaModule is yet another possible way to avoid putting
knowledge about QAPISchemaModularCVisitor's doings into
QAPISchemaModule.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 07/17] qapi/gen: Replace ._begin_system_module()
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (5 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 06/17] qapi: centralize is_[user|system|builtin]_module methods John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 08/17] qapi: use explicitly internal module names John Snow
` (9 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
From: Markus Armbruster <armbru@redhat.com>
QAPISchemaModularCVisitor._begin_system_module() is actually just for
the builtin module. Rename it to ._begin_builtin_module() and drop
its useless @name parameter.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 4 ++--
scripts/qapi/types.py | 2 +-
scripts/qapi/visit.py | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 3b7eddc1c21..9f850d23646 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -295,7 +295,7 @@ def write(self, output_dir: str, opt_builtins: bool = False) -> None:
genc.write(output_dir)
genh.write(output_dir)
- def _begin_system_module(self, name: None) -> None:
+ def _begin_builtin_module(self) -> None:
pass
def _begin_user_module(self, name: str) -> None:
@@ -305,7 +305,7 @@ def visit_module(self, module: QAPISchemaModule) -> None:
if module.system_module:
if self._builtin_blurb:
self._add_system_module(None, self._builtin_blurb)
- self._begin_system_module(module.name)
+ self._begin_builtin_module()
else:
# The built-in module has not been created. No code may
# be generated.
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2b4916cdaa1..dbf58c0b91d 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -271,7 +271,7 @@ def __init__(self, prefix: str):
prefix, 'qapi-types', ' * Schema-defined QAPI types',
' * Built-in QAPI types', __doc__)
- def _begin_system_module(self, name: None) -> None:
+ def _begin_builtin_module(self) -> None:
self._genc.preamble_add(mcgen('''
#include "qemu/osdep.h"
#include "qapi/dealloc-visitor.h"
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 339f1521524..568ba35592c 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -305,7 +305,7 @@ def __init__(self, prefix: str):
prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
' * Built-in QAPI visitors', __doc__)
- def _begin_system_module(self, name: None) -> None:
+ def _begin_builtin_module(self) -> None:
self._genc.preamble_add(mcgen('''
#include "qemu/osdep.h"
#include "qapi/error.h"
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 08/17] qapi: use explicitly internal module names
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (6 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 07/17] qapi/gen: Replace ._begin_system_module() John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 09/17] qapi: use './builtin' as the built-in module name John Snow
` (8 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
Use ./emit and ./init explicitly instead of "emit" and "init". Add
assertions that enforce this.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/commands.py | 2 +-
scripts/qapi/events.py | 2 +-
scripts/qapi/gen.py | 3 ++-
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 71744f48a35..fc5fe27c472 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_system_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 9851653b9d1..26faa829898 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_system_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 9f850d23646..18b72dd06c8 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -285,7 +285,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
self._add_module(name, blurb)
def _add_system_module(self, name: Optional[str], blurb: str) -> None:
- self._add_module(name and './' + name, blurb)
+ 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:
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 09/17] qapi: use './builtin' as the built-in module name
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (7 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 08/17] qapi: use explicitly internal module names John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-20 14:14 ` Markus Armbruster
2021-01-19 18:02 ` [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module John Snow
` (7 subsequent siblings)
16 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
Use './builtin' as the built-in module name instead of
None. Clarify the typing that this is now always a string.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 18 +++++++++---------
scripts/qapi/schema.py | 14 +++++++-------
| 2 +-
tests/qapi-schema/doc-good.out | 2 +-
tests/qapi-schema/empty.out | 2 +-
tests/qapi-schema/event-case.out | 2 +-
tests/qapi-schema/include-repetition.out | 2 +-
tests/qapi-schema/include-simple.out | 2 +-
tests/qapi-schema/indented-expr.out | 2 +-
tests/qapi-schema/qapi-schema-test.out | 2 +-
10 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 18b72dd06c8..55acd7e080d 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -246,16 +246,16 @@ def __init__(self,
self._pydoc = pydoc
self._genc: Optional[QAPIGenC] = None
self._genh: Optional[QAPIGenH] = None
- self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
+ self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
self._main_module: Optional[str] = None
@staticmethod
- def _module_dirname(name: Optional[str]) -> str:
+ def _module_dirname(name: str) -> str:
if QAPISchemaModule.is_user_module(name):
return os.path.dirname(name)
return ''
- def _module_basename(self, what: str, name: Optional[str]) -> str:
+ def _module_basename(self, what: str, name: str) -> str:
ret = '' if QAPISchemaModule.is_builtin_module(name) else self._prefix
if QAPISchemaModule.is_user_module(name):
basename = os.path.basename(name)
@@ -263,15 +263,15 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
if name != self._main_module:
ret += '-' + os.path.splitext(basename)[0]
else:
- name = name[2:] if name else 'builtin'
- ret += re.sub(r'-', '-' + name + '-', what)
+ assert QAPISchemaModule.is_system_module(name)
+ ret += re.sub(r'-', '-' + name[2:] + '-', what)
return ret
- def _module_filename(self, what: str, name: Optional[str]) -> str:
+ def _module_filename(self, what: str, name: str) -> str:
return os.path.join(self._module_dirname(name),
self._module_basename(what, name))
- def _add_module(self, name: Optional[str], blurb: str) -> None:
+ def _add_module(self, name: str, blurb: str) -> None:
basename = self._module_filename(self._what, name)
genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
@@ -284,7 +284,7 @@ def _add_user_module(self, name: str, blurb: str) -> None:
self._main_module = name
self._add_module(name, blurb)
- def _add_system_module(self, name: Optional[str], blurb: str) -> None:
+ def _add_system_module(self, name: str, blurb: str) -> None:
assert QAPISchemaModule.is_system_module(name)
self._add_module(name, blurb)
@@ -305,7 +305,7 @@ def _begin_user_module(self, name: str) -> None:
def visit_module(self, module: QAPISchemaModule) -> None:
if module.system_module:
if self._builtin_blurb:
- self._add_system_module(None, self._builtin_blurb)
+ self._add_system_module(module.name, self._builtin_blurb)
self._begin_builtin_module()
else:
# The built-in module has not been created. No code may
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 44a9232c881..c519c18a5ec 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -68,7 +68,8 @@ def check_doc(self):
def _set_module(self, schema, info):
assert self._checked
- self._module = schema.module_by_fname(info and info.fname)
+ fname = info.fname if info else './builtin'
+ self._module = schema.module_by_fname(fname)
self._module.add_entity(self)
def set_module(self, schema):
@@ -142,11 +143,11 @@ def __init__(self, name):
self._entity_list = []
@classmethod
- def is_system_module(cls, name: Optional[str]) -> bool:
- return name is None or name.startswith('./')
+ def is_system_module(cls, name: str) -> bool:
+ return name.startswith('./')
@classmethod
- def is_user_module(cls, name: Optional[str]) -> bool:
+ def is_user_module(cls, name: str) -> bool:
return not cls.is_system_module(name)
@classmethod
@@ -849,7 +850,7 @@ def __init__(self, fname):
self._entity_dict = {}
self._module_dict = OrderedDict()
self._schema_dir = os.path.dirname(fname)
- self._make_module(None) # built-ins
+ self._make_module('./builtin') # built-ins
self._make_module(fname)
self._predefining = True
self._def_predefineds()
@@ -894,7 +895,7 @@ def resolve_type(self, name, info, what):
info, "%s uses unknown type '%s'" % (what, name))
return typ
- def _module_name(self, fname):
+ def _module_name(self, fname: str) -> str:
if QAPISchemaModule.is_system_module(fname):
return fname
return os.path.relpath(fname, self._schema_dir)
@@ -907,7 +908,6 @@ def _make_module(self, fname):
def module_by_fname(self, fname):
name = self._module_name(fname)
- assert name in self._module_dict
return self._module_dict[name]
def _def_include(self, expr, info, doc):
--git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index 273f0f54e16..ce4f6a4f0f5 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,4 +1,4 @@
-module None
+module ./builtin
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 419284dae29..715b0bbc1af 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -1,4 +1,4 @@
-module None
+module ./builtin
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 69666c39ad2..3feb3f69d3d 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -1,4 +1,4 @@
-module None
+module ./builtin
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index 42ae519656d..9ae44052ac4 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,4 +1,4 @@
-module None
+module ./builtin
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out
index 0b654ddebb6..16dbd9b8194 100644
--- a/tests/qapi-schema/include-repetition.out
+++ b/tests/qapi-schema/include-repetition.out
@@ -1,4 +1,4 @@
-module None
+module ./builtin
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/include-simple.out b/tests/qapi-schema/include-simple.out
index 061f81e5090..48e923bfbc8 100644
--- a/tests/qapi-schema/include-simple.out
+++ b/tests/qapi-schema/include-simple.out
@@ -1,4 +1,4 @@
-module None
+module ./builtin
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 04356775cd1..6a30ded3fa6 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,4 +1,4 @@
-module None
+module ./builtin
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 8868ca0dca9..3b1387d9f18 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,4 +1,4 @@
-module None
+module ./builtin
object q_empty
enum QType
prefix QTYPE
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 09/17] qapi: use './builtin' as the built-in module name
2021-01-19 18:02 ` [PATCH v3 09/17] qapi: use './builtin' as the built-in module name John Snow
@ 2021-01-20 14:14 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2021-01-20 14:14 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Use './builtin' as the built-in module name instead of
> None. Clarify the typing that this is now always a string.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/gen.py | 18 +++++++++---------
> scripts/qapi/schema.py | 14 +++++++-------
> tests/qapi-schema/comments.out | 2 +-
> tests/qapi-schema/doc-good.out | 2 +-
> tests/qapi-schema/empty.out | 2 +-
> tests/qapi-schema/event-case.out | 2 +-
> tests/qapi-schema/include-repetition.out | 2 +-
> tests/qapi-schema/include-simple.out | 2 +-
> tests/qapi-schema/indented-expr.out | 2 +-
> tests/qapi-schema/qapi-schema-test.out | 2 +-
> 10 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 18b72dd06c8..55acd7e080d 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -246,16 +246,16 @@ def __init__(self,
> self._pydoc = pydoc
> self._genc: Optional[QAPIGenC] = None
> self._genh: Optional[QAPIGenH] = None
> - self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
> + self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
> self._main_module: Optional[str] = None
>
> @staticmethod
> - def _module_dirname(name: Optional[str]) -> str:
> + def _module_dirname(name: str) -> str:
> if QAPISchemaModule.is_user_module(name):
> return os.path.dirname(name)
> return ''
>
> - def _module_basename(self, what: str, name: Optional[str]) -> str:
> + def _module_basename(self, what: str, name: str) -> str:
> ret = '' if QAPISchemaModule.is_builtin_module(name) else self._prefix
> if QAPISchemaModule.is_user_module(name):
> basename = os.path.basename(name)
> @@ -263,15 +263,15 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
> if name != self._main_module:
> ret += '-' + os.path.splitext(basename)[0]
> else:
> - name = name[2:] if name else 'builtin'
> - ret += re.sub(r'-', '-' + name + '-', what)
> + assert QAPISchemaModule.is_system_module(name)
> + ret += re.sub(r'-', '-' + name[2:] + '-', what)
> return ret
>
> - def _module_filename(self, what: str, name: Optional[str]) -> str:
> + def _module_filename(self, what: str, name: str) -> str:
> return os.path.join(self._module_dirname(name),
> self._module_basename(what, name))
>
> - def _add_module(self, name: Optional[str], blurb: str) -> None:
> + def _add_module(self, name: str, blurb: str) -> None:
> basename = self._module_filename(self._what, name)
> genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
> genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> @@ -284,7 +284,7 @@ def _add_user_module(self, name: str, blurb: str) -> None:
> self._main_module = name
> self._add_module(name, blurb)
>
> - def _add_system_module(self, name: Optional[str], blurb: str) -> None:
> + def _add_system_module(self, name: str, blurb: str) -> None:
> assert QAPISchemaModule.is_system_module(name)
> self._add_module(name, blurb)
>
> @@ -305,7 +305,7 @@ def _begin_user_module(self, name: str) -> None:
> def visit_module(self, module: QAPISchemaModule) -> None:
> if module.system_module:
> if self._builtin_blurb:
> - self._add_system_module(None, self._builtin_blurb)
> + self._add_system_module(module.name, self._builtin_blurb)
> self._begin_builtin_module()
> else:
> # The built-in module has not been created. No code may
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 44a9232c881..c519c18a5ec 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -68,7 +68,8 @@ def check_doc(self):
>
> def _set_module(self, schema, info):
> assert self._checked
> - self._module = schema.module_by_fname(info and info.fname)
> + fname = info.fname if info else './builtin'
> + self._module = schema.module_by_fname(fname)
> self._module.add_entity(self)
>
> def set_module(self, schema):
> @@ -142,11 +143,11 @@ def __init__(self, name):
> self._entity_list = []
>
> @classmethod
> - def is_system_module(cls, name: Optional[str]) -> bool:
> - return name is None or name.startswith('./')
> + def is_system_module(cls, name: str) -> bool:
> + return name.startswith('./')
>
> @classmethod
> - def is_user_module(cls, name: Optional[str]) -> bool:
> + def is_user_module(cls, name: str) -> bool:
> return not cls.is_system_module(name)
>
> @classmethod
> @@ -849,7 +850,7 @@ def __init__(self, fname):
> self._entity_dict = {}
> self._module_dict = OrderedDict()
> self._schema_dir = os.path.dirname(fname)
> - self._make_module(None) # built-ins
> + self._make_module('./builtin') # built-ins
The comment it now redundant.
> self._make_module(fname)
> self._predefining = True
> self._def_predefineds()
> @@ -894,7 +895,7 @@ def resolve_type(self, name, info, what):
> info, "%s uses unknown type '%s'" % (what, name))
> return typ
>
> - def _module_name(self, fname):
> + def _module_name(self, fname: str) -> str:
> if QAPISchemaModule.is_system_module(fname):
> return fname
> return os.path.relpath(fname, self._schema_dir)
> @@ -907,7 +908,6 @@ def _make_module(self, fname):
>
> def module_by_fname(self, fname):
> name = self._module_name(fname)
> - assert name in self._module_dict
> return self._module_dict[name]
>
> def _def_include(self, expr, info, doc):
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (8 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 09/17] qapi: use './builtin' as the built-in module name John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-20 14:20 ` Markus Armbruster
2021-01-19 18:02 ` [PATCH v3 11/17] qapi: centralize the built-in module name definition John Snow
` (6 subsequent siblings)
16 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module
2021-01-19 18:02 ` [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module John Snow
@ 2021-01-20 14:20 ` Markus Armbruster
2021-01-20 16:10 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2021-01-20 14:20 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
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:
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module
2021-01-20 14:20 ` Markus Armbruster
@ 2021-01-20 16:10 ` John Snow
2021-01-21 7:40 ` Markus Armbruster
0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2021-01-20 16:10 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
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:
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module
2021-01-20 16:10 ` John Snow
@ 2021-01-21 7:40 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2021-01-21 7:40 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
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:
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 11/17] qapi: centralize the built-in module name definition
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (9 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 12/17] qapi/gen: write _genc/_genh access shims John Snow
` (5 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
Use a constant to make it obvious we're referring to a very specific thing.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/schema.py | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index c519c18a5ec..887a1c82c6f 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -68,7 +68,7 @@ def check_doc(self):
def _set_module(self, schema, info):
assert self._checked
- fname = info.fname if info else './builtin'
+ fname = info.fname if info else QAPISchemaModule.BUILTIN_MODULE_NAME
self._module = schema.module_by_fname(fname)
self._module.add_entity(self)
@@ -138,6 +138,9 @@ def visit_event(self, name, info, ifcond, features, arg_type, boxed):
class QAPISchemaModule:
+
+ BUILTIN_MODULE_NAME = './builtin'
+
def __init__(self, name):
self.name = name
self._entity_list = []
@@ -152,7 +155,7 @@ def is_user_module(cls, name: str) -> bool:
@classmethod
def is_builtin_module(cls, name: str) -> bool:
- return name == './builtin'
+ return name == cls.BUILTIN_MODULE_NAME
@property
def system_module(self) -> bool:
@@ -850,7 +853,7 @@ def __init__(self, fname):
self._entity_dict = {}
self._module_dict = OrderedDict()
self._schema_dir = os.path.dirname(fname)
- self._make_module('./builtin') # built-ins
+ self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME) # built-ins
self._make_module(fname)
self._predefining = True
self._def_predefineds()
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 12/17] qapi/gen: write _genc/_genh access shims
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (10 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 11/17] qapi: centralize the built-in module name definition John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 13/17] qapi/gen: Support for switching to another module temporarily John Snow
` (4 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
Many places assume they can access these fields without checking them
first to ensure they are defined. Eliminating the _genc and _genh fields
and replacing them with functional properties that check for correct
state can ease the typing overhead by eliminating the Optional[T] return
type.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b5505685e6e..4b5d3ea3a5b 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -244,11 +244,20 @@ def __init__(self,
self._user_blurb = user_blurb
self._builtin_blurb = builtin_blurb
self._pydoc = pydoc
- self._genc: Optional[QAPIGenC] = None
- self._genh: Optional[QAPIGenH] = None
+ self._current_module: Optional[str] = None
self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
self._main_module: Optional[str] = None
+ @property
+ def _genc(self) -> QAPIGenC:
+ assert self._current_module is not None
+ return self._module[self._current_module][0]
+
+ @property
+ def _genh(self) -> QAPIGenH:
+ assert self._current_module is not None
+ return self._module[self._current_module][1]
+
@staticmethod
def _module_dirname(name: str) -> str:
if QAPISchemaModule.is_user_module(name):
@@ -279,7 +288,7 @@ def _add_module(self, name: str, blurb: str) -> None:
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]
+ self._current_module = name
def write(self, output_dir: str, opt_builtins: bool = False) -> None:
for name in self._module:
@@ -303,8 +312,7 @@ def visit_module(self, module: QAPISchemaModule) -> None:
else:
# The built-in module has not been created. No code may
# be generated.
- self._genc = None
- self._genh = None
+ self._current_module = None
else:
assert module.user_module, "Unexpected system module"
self._add_module(module.name, self._user_blurb)
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 13/17] qapi/gen: Support for switching to another module temporarily
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (11 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 12/17] qapi/gen: write _genc/_genh access shims John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 14/17] qapi/commands: Simplify command registry generation John Snow
` (3 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
From: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 4b5d3ea3a5b..86abdabb57a 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -290,6 +290,13 @@ def _add_module(self, name: str, blurb: str) -> None:
self._module[name] = (genc, genh)
self._current_module = name
+ @contextmanager
+ def _temp_module(self, name: str) -> Iterator[None]:
+ old_module = self._current_module
+ self._current_module = name
+ yield
+ self._current_module = old_module
+
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:
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 14/17] qapi/commands: Simplify command registry generation
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (12 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 13/17] qapi/gen: Support for switching to another module temporarily John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 15/17] qapi/gen: Drop support for QAPIGen without a file name John Snow
` (2 subsequent siblings)
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
From: Markus Armbruster <armbru@redhat.com>
QAPISchemaGenCommandVisitor.visit_command() needs to generate the
marshalling function into the current module, and also generate its
registration into the ./init system module. The latter is done
somewhat awkwardly: .__init__() creates a QAPIGenCCode that will not
be written out, each .visit_command() adds its registration to it, and
.visit_end() copies its contents into the ./init module it creates.
Instead provide the means to temporarily switch to another module.
Create the ./init module in .visit_begin(), and generate its initial
part. Add registrations to it in .visit_command(). Finish it in
.visit_end().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/commands.py | 49 ++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 49111663394..13a9dcaf894 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -23,7 +23,6 @@
from .common import c_name, mcgen
from .gen import (
QAPIGenC,
- QAPIGenCCode,
QAPISchemaModularCVisitor,
build_params,
ifcontext,
@@ -237,28 +236,11 @@ def gen_register_command(name: str,
return ret
-def gen_registry(registry: str, prefix: str) -> str:
- ret = mcgen('''
-
-void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
-{
- QTAILQ_INIT(cmds);
-
-''',
- c_prefix=c_name(prefix, protect=False))
- ret += registry
- ret += mcgen('''
-}
-''')
- return ret
-
-
class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
def __init__(self, prefix: str):
super().__init__(
prefix, 'qapi-commands',
' * Schema-defined QAPI/QMP commands', None, __doc__)
- self._regy = QAPIGenCCode(None)
self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
def _begin_user_module(self, name: str) -> None:
@@ -285,7 +267,7 @@ def _begin_user_module(self, name: str) -> None:
''',
types=types))
- def visit_end(self) -> None:
+ def visit_begin(self, schema: QAPISchema) -> None:
self._add_module('./init', ' * QAPI Commands initialization')
self._genh.add(mcgen('''
#include "qapi/qmp/dispatch.h"
@@ -293,13 +275,24 @@ def visit_end(self) -> None:
void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
''',
c_prefix=c_name(self._prefix, protect=False)))
- self._genc.preamble_add(mcgen('''
+ self._genc.add(mcgen('''
#include "qemu/osdep.h"
#include "%(prefix)sqapi-commands.h"
#include "%(prefix)sqapi-init-commands.h"
+
+void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
+{
+ QTAILQ_INIT(cmds);
+
''',
- prefix=self._prefix))
- self._genc.add(gen_registry(self._regy.get_content(), self._prefix))
+ prefix=self._prefix,
+ c_prefix=c_name(self._prefix, protect=False)))
+
+ def visit_end(self) -> None:
+ with self._temp_module('./init'):
+ self._genc.add(mcgen('''
+}
+'''))
def visit_command(self,
name: str,
@@ -324,15 +317,17 @@ def visit_command(self,
if ret_type and ret_type not in self._visited_ret_types[self._genc]:
self._visited_ret_types[self._genc].add(ret_type)
with ifcontext(ret_type.ifcond,
- self._genh, self._genc, self._regy):
+ self._genh, self._genc):
self._genc.add(gen_marshal_output(ret_type))
- with ifcontext(ifcond, self._genh, self._genc, self._regy):
+ with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
self._genh.add(gen_marshal_decl(name))
self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
- self._regy.add(gen_register_command(name, success_response,
- allow_oob, allow_preconfig,
- coroutine))
+ with self._temp_module('./init'):
+ with ifcontext(ifcond, self._genh, self._genc):
+ self._genc.add(gen_register_command(name, success_response,
+ allow_oob, allow_preconfig,
+ coroutine))
def gen_commands(schema: QAPISchema,
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 15/17] qapi/gen: Drop support for QAPIGen without a file name
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (13 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 14/17] qapi/commands: Simplify command registry generation John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 16/17] qapi: type 'info' as Optional[QAPISourceInfo] John Snow
2021-01-19 18:02 ` [PATCH v3 17/17] qapi: enable strict-optional checks John Snow
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
From: Markus Armbruster <armbru@redhat.com>
The previous commit removed the only user of QAPIGen(None). Tighten
the type hint.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 86abdabb57a..87074335e6c 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -40,7 +40,7 @@
class QAPIGen:
- def __init__(self, fname: Optional[str]):
+ def __init__(self, fname: str):
self.fname = fname
self._preamble = ''
self._body = ''
@@ -125,7 +125,7 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
class QAPIGenCCode(QAPIGen):
- def __init__(self, fname: Optional[str]):
+ def __init__(self, fname: str):
super().__init__(fname)
self._start_if: Optional[Tuple[List[str], str, str]] = None
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 16/17] qapi: type 'info' as Optional[QAPISourceInfo]
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (14 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 15/17] qapi/gen: Drop support for QAPIGen without a file name John Snow
@ 2021-01-19 18:02 ` John Snow
2021-01-19 18:02 ` [PATCH v3 17/17] qapi: enable strict-optional checks John Snow
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
For everything typed so far, type this parameter as Optional[QAPISourceInfo].
In the most generic case, QAPISchemaEntity's info field may be None to
represent types that come from built-in definitions. Although some
Entity types may not currently have any built-in definitions, it is not
easily possible to constrain the type except on an ad-hoc basis using
assertions.
It's easier and simpler, then, to just say it's always an Optional type.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/commands.py | 2 +-
scripts/qapi/events.py | 2 +-
scripts/qapi/gen.py | 2 +-
scripts/qapi/types.py | 2 +-
scripts/qapi/visit.py | 4 ++--
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 13a9dcaf894..54af519f44d 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -296,7 +296,7 @@ def visit_end(self) -> None:
def visit_command(self,
name: str,
- info: QAPISourceInfo,
+ info: Optional[QAPISourceInfo],
ifcond: List[str],
features: List[QAPISchemaFeature],
arg_type: Optional[QAPISchemaObjectType],
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 079c666ec69..8c57deb2b89 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -213,7 +213,7 @@ def visit_end(self) -> None:
def visit_event(self,
name: str,
- info: QAPISourceInfo,
+ info: Optional[QAPISourceInfo],
ifcond: List[str],
features: List[QAPISchemaFeature],
arg_type: Optional[QAPISchemaObjectType],
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 87074335e6c..f3f4d2b011e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -325,7 +325,7 @@ def visit_module(self, module: QAPISchemaModule) -> None:
self._add_module(module.name, self._user_blurb)
self._begin_user_module(module.name)
- def visit_include(self, name: str, info: QAPISourceInfo) -> None:
+ def visit_include(self, name: str, info: Optional[QAPISourceInfo]) -> None:
relname = os.path.relpath(self._module_filename(self._what, name),
os.path.dirname(self._genh.fname))
self._genh.preamble_add(mcgen('''
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index dbf58c0b91d..2bdd6268476 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -350,7 +350,7 @@ def visit_object_type(self,
def visit_alternate_type(self,
name: str,
- info: QAPISourceInfo,
+ info: Optional[QAPISourceInfo],
ifcond: List[str],
features: List[QAPISchemaFeature],
variants: QAPISchemaVariants) -> None:
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 568ba35592c..22e62df9017 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -336,7 +336,7 @@ def _begin_user_module(self, name: str) -> None:
def visit_enum_type(self,
name: str,
- info: QAPISourceInfo,
+ info: Optional[QAPISourceInfo],
ifcond: List[str],
features: List[QAPISchemaFeature],
members: List[QAPISchemaEnumMember],
@@ -378,7 +378,7 @@ def visit_object_type(self,
def visit_alternate_type(self,
name: str,
- info: QAPISourceInfo,
+ info: Optional[QAPISourceInfo],
ifcond: List[str],
features: List[QAPISchemaFeature],
variants: QAPISchemaVariants) -> None:
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 17/17] qapi: enable strict-optional checks
2021-01-19 18:02 [PATCH v3 00/17] qapi: static typing conversion, pt1.5 John Snow
` (15 preceding siblings ...)
2021-01-19 18:02 ` [PATCH v3 16/17] qapi: type 'info' as Optional[QAPISourceInfo] John Snow
@ 2021-01-19 18:02 ` John Snow
16 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2021-01-19 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, John Snow, Markus Armbruster,
Eduardo Habkost, Cleber Rosa
In the modules that we are checking so far, we can be stricter about the
difference between Optional[T] and T types. Enable that check.
Enabling it now will assist review on further typing and cleanup work.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/mypy.ini | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 74fc6c82153..04bd5db5278 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -1,6 +1,5 @@
[mypy]
strict = True
-strict_optional = False
disallow_untyped_calls = False
python_version = 3.6
--
2.26.2
^ permalink raw reply related [flat|nested] 29+ messages in thread