Instead of using None as the built-in module filename, use an empty
string instead. This allows us to clarify the type of various interfaces
dealing with module names as always taking a string, which saves us from
having to use Optional[str] everywhere.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 6 +++---
scripts/qapi/schema.py | 12 ++++++------
scripts/qapi/types.py | 2 +-
scripts/qapi/visit.py | 2 +-
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 +-
12 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b2c89213d838..a577a4a7f002 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -309,14 +309,14 @@ 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_system_module(self, name: str) -> None:
pass
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, name: str) -> None:
+ if not name:
if self._builtin_blurb:
self._add_system_module('builtin', self._builtin_blurb)
self._begin_system_module(name)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index d5f19732b516..8d8b8758f65e 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -69,7 +69,7 @@ def check_doc(self):
def _set_module(self, schema, info):
assert self._checked
- self._module = schema.module_by_fname(info and info.fname)
+ self._module = schema.module_by_fname(info.fname)
self._module.add_entity(self)
def set_module(self, schema):
@@ -826,7 +826,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(QAPISourceInfo.builtin().fname) # built-ins
self._make_module(fname)
self._predefining = True
self._def_predefineds()
@@ -871,10 +871,10 @@ def resolve_type(self, name, info, what):
info, "%s uses unknown type '%s'" % (what, name))
return typ
- def _module_name(self, fname):
- if not fname:
- return None
- return os.path.relpath(fname, self._schema_dir)
+ def _module_name(self, fname: str) -> str:
+ if fname:
+ return os.path.relpath(fname, self._schema_dir)
+ return fname
def _make_module(self, fname):
name = self._module_name(fname)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index a3a16284006b..12eeea3aaffe 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -272,7 +272,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_system_module(self, name: str) -> 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 3f49c307c566..76e34ee7f02e 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_system_module(self, name: str) -> None:
self._genc.preamble_add(mcgen('''
#include "qemu/osdep.h"
#include "qapi/error.h"
diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index 08aba8354e2a..02000c06e5e0 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 83a3d9bd69b5..494533d74793 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 0dac23c80c15..059caa4e1d2a 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index ace511ba5a96..4d9d2b519f4b 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out
index f7ab4987943c..31d64631b665 100644
--- a/tests/qapi-schema/include-repetition.out
+++ b/tests/qapi-schema/include-repetition.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/include-simple.out b/tests/qapi-schema/include-simple.out
index 81bdeb887b66..1b35b3295713 100644
--- a/tests/qapi-schema/include-simple.out
+++ b/tests/qapi-schema/include-simple.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 361a58185e67..aed689e7bd67 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
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 4f5ab9fd596c..4a899ba63ecb 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
--
2.26.2
John Snow <jsnow@redhat.com> writes: > Instead of using None as the built-in module filename, use an empty > string instead. PATCH 05's changes the module name of the special system module for built-in stuff from None to './builtin'. The other system modules are named like './FOO': './init' and './emit' currently. This one changes the module *filename* from None to "", and not just for the builtin module, but for *all* system modules. Your commit message claims only "the built-in module", which is not true as far as I can tell. Should we use the opportunity to make the filename match the module name? > This allows us to clarify the type of various interfaces > dealing with module names as always taking a string, which saves us from > having to use Optional[str] everywhere. > > Signed-off-by: John Snow <jsnow@redhat.com>
On 12/16/20 5:42 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Instead of using None as the built-in module filename, use an empty
>> string instead.
>
> PATCH 05's changes the module name of the special system module for
> built-in stuff from None to './builtin'. The other system modules are
> named like './FOO': './init' and './emit' currently.
>
> This one changes the module *filename* from None to "", and not just for
> the builtin module, but for *all* system modules. Your commit message
> claims only "the built-in module", which is not true as far as I can
> tell.
>
Is this true? ... "./init" and "./emit" are defined only in the
generators, outside of the schema entirely. They don't even have
"QAPISchemaModule" objects associated with them.
I changed:
self._make_module(None) # built-ins
to
self._make_module(QAPISourceInfo.builtin().fname) # built-ins
that should be precisely only "the" built-in module, shouldn't it? the
other "system" modules don't even pass through _make_module.
> Should we use the opportunity to make the filename match the module
> name?
>
If that's something you want to have happen, it can be done, yes.
I had a draft that did it that way initially; I was afraid I was mixing
up two distinct things: the module fname (schema.py's concept of
modules) and module name (gen.py's concept of modules). This version of
my patch kept the two more distinct like they are currently.
We can change "the" built-in module to have an fname of "./builtin"
which works just fine; gen.py just has to change to not add "./" to
modules already declared with the leading slash.
Up for debate is if you want the system modules declared in the code
generators to have to declare 'emit' or './emit'; I left them alone and
allowed them to say 'event'.
Downside: the ./ prefix takes on special meaning in both gen.py and
schema.py. the module organization feels decentralized and fragile.
>> This allows us to clarify the type of various interfaces
>> dealing with module names as always taking a string, which saves us from
>> having to use Optional[str] everywhere.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
John Snow <jsnow@redhat.com> writes:
> On 12/16/20 5:42 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Instead of using None as the built-in module filename, use an empty
>>> string instead.
>>
>> PATCH 05's changes the module name of the special system module for
>> built-in stuff from None to './builtin'. The other system modules are
>> named like './FOO': './init' and './emit' currently.
>>
>> This one changes the module *filename* from None to "", and not just for
>> the builtin module, but for *all* system modules. Your commit message
>> claims only "the built-in module", which is not true as far as I can
>> tell.
>>
>
> Is this true? ... "./init" and "./emit" are defined only in the
> generators, outside of the schema entirely. They don't even have
> "QAPISchemaModule" objects associated with them.
>
> I changed:
>
> self._make_module(None) # built-ins
>
>
> to
>
> self._make_module(QAPISourceInfo.builtin().fname) # built-ins
>
>
>
> that should be precisely only "the" built-in module, shouldn't it? the
> other "system" modules don't even pass through _make_module.
You're right.
The schema IR has only user-defined modules and the built-ins module.
Each module has a name. We use the source file name for the
user-defined modules. The built-ins module has none, so we use None.
The QAPISchemaModularCVisitor generates "modular" output. It has
per-module data, keyed by module name. It supports user-defined and
system modules. We set them up as follows. The user-defined modules
are exactly the schema IR's (same name). The system modules are the
schema IR's built-ins module (same name) plus whatever the user of
QAPISchemaModularCVisitor adds (convention: name starts with ./, so it
can't clash).
Why let generators add system modules that don't exist in the schema IR?
It's a reasonably clean way to generate stuff that doesn't fit elsewhere
into separate .[ch] files.
PATCH 05 changes the name of the built-ins module from None to
'./builtins' in the QAPISchemaModularCVisitor only. Correct?
This patch changes the name of the built-ins module from None to "" in
the schema IR only. Correct?
>> Should we use the opportunity to make the filename match the module
>> name?
>>
>
> If that's something you want to have happen, it can be done, yes.
Having different "module names" for the built-ins module in different
places could be confusing.
The issue appears in PATCH 05. I'm fine with the two module names
diverging temporarily in this series. I'd prefer them to be the same at
the end.
> I had a draft that did it that way initially; I was afraid I was mixing
> up two distinct things: the module fname (schema.py's concept of
> modules) and module name (gen.py's concept of modules). This version of
> my patch kept the two more distinct like they are currently.
>
> We can change "the" built-in module to have an fname of "./builtin"
> which works just fine; gen.py just has to change to not add "./" to
> modules already declared with the leading slash.
>
> Up for debate is if you want the system modules declared in the code
> generators to have to declare 'emit' or './emit'; I left them alone and
> allowed them to say 'event'.
I pass 'emit' to argument name, so a simple './' + name obviously
results in a system module name.
With './emit', we achieve "obviously" by assert name.startswith('./').
Matter of taste.
> Downside: the ./ prefix takes on special meaning in both gen.py and
> schema.py. the module organization feels decentralized and fragile.
Making the './' prefix special is fine. But you're right, doing it in
two places and with precious little explanation is not so fine.
We could have schema.py provide some notion of "module name". Weasel
words "some notion" for artistic license.
If we'd prefer not to do it now, a few judicious comments should suffice
for now.
>>> This allows us to clarify the type of various interfaces
>>> dealing with module names as always taking a string, which saves us from
>>> having to use Optional[str] everywhere.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
On 12/17/20 6:09 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 12/16/20 5:42 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Instead of using None as the built-in module filename, use an empty
>>>> string instead.
>>>
>>> PATCH 05's changes the module name of the special system module for
>>> built-in stuff from None to './builtin'. The other system modules are
>>> named like './FOO': './init' and './emit' currently.
>>>
>>> This one changes the module *filename* from None to "", and not just for
>>> the builtin module, but for *all* system modules. Your commit message
>>> claims only "the built-in module", which is not true as far as I can
>>> tell.
>>>
>>
>> Is this true? ... "./init" and "./emit" are defined only in the
>> generators, outside of the schema entirely. They don't even have
>> "QAPISchemaModule" objects associated with them.
>>
>> I changed:
>>
>> self._make_module(None) # built-ins
>>
>>
>> to
>>
>> self._make_module(QAPISourceInfo.builtin().fname) # built-ins
>>
>>
>>
>> that should be precisely only "the" built-in module, shouldn't it? the
>> other "system" modules don't even pass through _make_module.
>
> You're right.
>
> The schema IR has only user-defined modules and the built-ins module.
> Each module has a name. We use the source file name for the
> user-defined modules. The built-ins module has none, so we use None.
>
> The QAPISchemaModularCVisitor generates "modular" output. It has
> per-module data, keyed by module name. It supports user-defined and
> system modules. We set them up as follows. The user-defined modules
> are exactly the schema IR's (same name). The system modules are the
> schema IR's built-ins module (same name) plus whatever the user of
> QAPISchemaModularCVisitor adds (convention: name starts with ./, so it
> can't clash).
>
> Why let generators add system modules that don't exist in the schema IR?
> It's a reasonably clean way to generate stuff that doesn't fit elsewhere
> into separate .[ch] files.
>
> PATCH 05 changes the name of the built-ins module from None to
> './builtins' in the QAPISchemaModularCVisitor only. Correct?
>
That's right. That was a useful change all by itself, and winds up being
useful for the genc/genh typing.
> This patch changes the name of the built-ins module from None to "" in
> the schema IR only. Correct?
>
As far as I know, yes. ("Schema IR -- Internal Registry?")
>>> Should we use the opportunity to make the filename match the module
>>> name?
>>>
>>
>> If that's something you want to have happen, it can be done, yes.
>
> Having different "module names" for the built-ins module in different
> places could be confusing.
>
Yes, but we technically didn't use the generator-only names in the
Schema before, so I didn't want to assume we'd want them here.
> The issue appears in PATCH 05. I'm fine with the two module names
> diverging temporarily in this series. I'd prefer them to be the same at
> the end.
>
OK, no problem. I'll try to make this nicer and unify things just a
pinch more.
--js
>> I had a draft that did it that way initially; I was afraid I was mixing
>> up two distinct things: the module fname (schema.py's concept of
>> modules) and module name (gen.py's concept of modules). This version of
>> my patch kept the two more distinct like they are currently.
>>
>> We can change "the" built-in module to have an fname of "./builtin"
>> which works just fine; gen.py just has to change to not add "./" to
>> modules already declared with the leading slash.
>>
>> Up for debate is if you want the system modules declared in the code
>> generators to have to declare 'emit' or './emit'; I left them alone and
>> allowed them to say 'event'.
>
> I pass 'emit' to argument name, so a simple './' + name obviously
> results in a system module name.
>
> With './emit', we achieve "obviously" by assert name.startswith('./').
> Matter of taste.
>
>> Downside: the ./ prefix takes on special meaning in both gen.py and
>> schema.py. the module organization feels decentralized and fragile.
>
> Making the './' prefix special is fine. But you're right, doing it in
> two places and with precious little explanation is not so fine.
>
> We could have schema.py provide some notion of "module name". Weasel
> words "some notion" for artistic license.
>
> If we'd prefer not to do it now, a few judicious comments should suffice
> for now.
>
>>>> This allows us to clarify the type of various interfaces
>>>> dealing with module names as always taking a string, which saves us from
>>>> having to use Optional[str] everywhere.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
John Snow <jsnow@redhat.com> writes:
> On 12/17/20 6:09 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 12/16/20 5:42 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> Instead of using None as the built-in module filename, use an empty
>>>>> string instead.
>>>>
>>>> PATCH 05's changes the module name of the special system module for
>>>> built-in stuff from None to './builtin'. The other system modules are
>>>> named like './FOO': './init' and './emit' currently.
>>>>
>>>> This one changes the module *filename* from None to "", and not just for
>>>> the builtin module, but for *all* system modules. Your commit message
>>>> claims only "the built-in module", which is not true as far as I can
>>>> tell.
>>>>
>>>
>>> Is this true? ... "./init" and "./emit" are defined only in the
>>> generators, outside of the schema entirely. They don't even have
>>> "QAPISchemaModule" objects associated with them.
>>>
>>> I changed:
>>>
>>> self._make_module(None) # built-ins
>>>
>>>
>>> to
>>>
>>> self._make_module(QAPISourceInfo.builtin().fname) # built-ins
>>>
>>>
>>>
>>> that should be precisely only "the" built-in module, shouldn't it? the
>>> other "system" modules don't even pass through _make_module.
>>
>> You're right.
>>
>> The schema IR has only user-defined modules and the built-ins module.
>> Each module has a name. We use the source file name for the
>> user-defined modules. The built-ins module has none, so we use None.
>>
>> The QAPISchemaModularCVisitor generates "modular" output. It has
>> per-module data, keyed by module name. It supports user-defined and
>> system modules. We set them up as follows. The user-defined modules
>> are exactly the schema IR's (same name). The system modules are the
>> schema IR's built-ins module (same name) plus whatever the user of
>> QAPISchemaModularCVisitor adds (convention: name starts with ./, so it
>> can't clash).
>>
>> Why let generators add system modules that don't exist in the schema IR?
>> It's a reasonably clean way to generate stuff that doesn't fit elsewhere
>> into separate .[ch] files.
>>
>> PATCH 05 changes the name of the built-ins module from None to
>> './builtins' in the QAPISchemaModularCVisitor only. Correct?
>>
>
> That's right. That was a useful change all by itself, and winds up being
> useful for the genc/genh typing.
>
>> This patch changes the name of the built-ins module from None to "" in
>> the schema IR only. Correct?
>>
>
> As far as I know, yes. ("Schema IR -- Internal Registry?")
Internal representation (the stuff in schema.py). Compiler jargon,
sorry.
>>>> Should we use the opportunity to make the filename match the module
>>>> name?
>>>>
>>>
>>> If that's something you want to have happen, it can be done, yes.
>>
>> Having different "module names" for the built-ins module in different
>> places could be confusing.
>>
>
> Yes, but we technically didn't use the generator-only names in the
> Schema before, so I didn't want to assume we'd want them here.
We can't have them there, because the things they name don't exist
there.
What we can have is a consistent naming convention that leads to same
things having the same names in both places.
>> The issue appears in PATCH 05. I'm fine with the two module names
>> diverging temporarily in this series. I'd prefer them to be the same at
>> the end.
>>
>
> OK, no problem. I'll try to make this nicer and unify things just a
> pinch more.
>
> --js
© 2016 - 2025 Red Hat, Inc.