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

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