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

[Eduardo Habkost]

On Fri, Sep 25, 2020 at 03:00:51PM +0200, 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.
> 
> Note that (1) and (2) are disjoint: relative pathnames do not begin with
> './'.
> 
> name='' is not possible, because '' is not a valid pathname.

Thanks!  So, the './' prefix is just internal state and never
visible to the outside, correct?  I would use a separate bool
instead of trying to encode additional state inside the string.

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

-- 
Eduardo