[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module |
Date: |
Fri, 25 Sep 2020 15:00:51 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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
>>
- [PATCH v2 20/38] qapi/commands.py: add notational type hints, (continued)
- Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module,
Markus Armbruster <=
- Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module, Eduardo Habkost, 2020/09/25
- Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module, Eduardo Habkost, 2020/09/25
- Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module, Markus Armbruster, 2020/09/28
- Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module, John Snow, 2020/09/25
Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module, Cleber Rosa, 2020/09/23
[PATCH v2 30/38] qapi/introspect.py: Add a typed 'extra' structure, John Snow, 2020/09/22