Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
>> The edge case is that if the name is '', this expression returns a
>> string instead of a bool, which violates our declared type.
>> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  scripts/qapi/gen.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index 9898d513ae..cb2b2655c3 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, 
>> builtin_blurb, pydoc):
>>  
>>      @staticmethod
>>      def _is_user_module(name):
>> -        return name and not name.startswith('./')
>> +        return name is not None and not name.startswith('./')
>
> This changes behavior if name=='', and I guess this is OK, but
> I'm not sure.

@name is either

(1) A module pathname relative to the main module

    This is a module defined by the user.

(2) system module name, starting with './'

    This is a named system module.  We currently have two: './init' in
    commands.py, and and './emit' in events.py.

(3) None

    This is the (nameless) system module for built-in stuff.  It
    predates (2).  Using './builtin' would probably be better now.

Note that (1) and (2) are disjoint: relative pathnames do not begin with
'./'.

name='' is not possible, because '' is not a valid pathname.

>                I miss documentation on `visit_module()`,
> `visit_include()`, and `_is_user_module()`.  I don't know what
> `name` means here, and what is a "user module".

Valid complaints!  The code is subtle in places, without helping its
readers along with comments or doc strings.

>
>>  
>>      @staticmethod
>>      def _is_builtin_module(name):
>> -- 
>> 2.26.2
>>