qemu-devel
[Top][All Lists]
Advanced

[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: John Snow
Subject: Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module
Date: Fri, 25 Sep 2020 12:29:14 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0

On 9/25/20 9:00 AM, Markus Armbruster wrote:
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.


Yes please! This would help simplify Optional[str] to str in many places, and removes doubt as to what "None" might imply.

Let's queue that idea up as a cleanup for after this typing series.

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


For now, I've done the simpler thing and wrapped the return in bool(...), but we will be able to do much more cleanups if we eliminate the possibility of "None" module names later. I'll get it all at once.

I'm adding it to my python TODO: https://gitlab.com/jsnow/qemu/-/issues/8

--js




reply via email to

[Prev in Thread] Current Thread [Next in Thread]