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

[Markus Armbruster]

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