Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None

[Markus Armbruster]

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>