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?
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>