From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>,
qemu-devel@nongnu.org, "Eduardo Habkost" <ehabkost@redhat.com>,
"Cleber Rosa" <crosa@redhat.com>
Subject: Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None
Date: Fri, 18 Dec 2020 06:31:20 +0100 [thread overview]
Message-ID: <87ft43d6jb.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <7c73d8da-b9ba-fa36-f923-067c0920c3ef@redhat.com> (John Snow's message of "Thu, 17 Dec 2020 16:07:37 -0500")
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
next prev parent reply other threads:[~2020-12-18 5:32 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
2020-12-14 23:53 ` [PATCH 01/12] qapi/commands: assert arg_type is not None John Snow
2020-12-14 23:53 ` [PATCH 02/12] qapi/events: fix visit_event typing John Snow
2020-12-14 23:53 ` [PATCH 03/12] qapi/main: handle theoretical None-return from re.match() John Snow
2020-12-16 8:23 ` Markus Armbruster
2020-12-16 17:11 ` John Snow
2020-12-14 23:53 ` [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond John Snow
2020-12-16 8:26 ` Markus Armbruster
2020-12-16 17:13 ` John Snow
2020-12-17 7:24 ` Markus Armbruster
2020-12-14 23:53 ` [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name John Snow
2020-12-16 8:35 ` Markus Armbruster
2020-12-16 17:27 ` John Snow
2020-12-14 23:53 ` [PATCH 06/12] qapi/source: Add builtin null-object sentinel John Snow
2020-12-16 9:22 ` Markus Armbruster
2020-12-16 17:53 ` John Snow
2020-12-17 12:33 ` Markus Armbruster
2020-12-18 19:14 ` John Snow
2020-12-16 19:11 ` John Snow
2020-12-17 11:56 ` Markus Armbruster
2020-12-18 19:22 ` John Snow
2020-12-14 23:53 ` [PATCH 07/12] qapi/gen: write _genc/_genh access shims John Snow
2020-12-14 23:53 ` [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory John Snow
2020-12-16 10:18 ` Markus Armbruster
2020-12-16 18:41 ` John Snow
2020-12-17 8:02 ` Markus Armbruster
2020-12-17 17:02 ` John Snow
2020-12-18 5:24 ` Markus Armbruster
2020-12-18 19:17 ` John Snow
2020-12-18 20:57 ` Markus Armbruster
2020-12-18 21:30 ` John Snow
2020-12-14 23:53 ` [PATCH 09/12] qapi/gen: move write method to QAPIGenC, make fname a str John Snow
2020-12-16 10:31 ` Markus Armbruster
2020-12-14 23:53 ` [PATCH 10/12] tests/qapi-schema: Add quotes to module name in test output John Snow
2020-12-14 23:53 ` [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None John Snow
2020-12-16 10:42 ` Markus Armbruster
2020-12-16 18:57 ` John Snow
2020-12-17 11:09 ` Markus Armbruster
2020-12-17 21:07 ` John Snow
2020-12-18 5:31 ` Markus Armbruster [this message]
2020-12-14 23:53 ` [PATCH 12/12] qapi: enable strict-optional checks John Snow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ft43d6jb.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=crosa@redhat.com \
--cc=ehabkost@redhat.com \
--cc=jsnow@redhat.com \
--cc=marcandre.lureau@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).