Re: [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name

[John Snow]

On 12/16/20 3:35 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > Use this in preference to 'None', which helps remove some edge cases in
> > the typing.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Clearly better.  Should've done it this way in commit c2e196a9b4 "qapi:
> Prepare for system modules other than 'builtin'".
>
> > ---
> >   scripts/qapi/gen.py | 27 +++++++++++++--------------
> >   1 file changed, 13 insertions(+), 14 deletions(-)
> >
> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > index a6dc991b1d03..0c5d1fee6088 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -245,23 +245,23 @@ def __init__(self,
> >           self._pydoc = pydoc
> >           self._genc: Optional[QAPIGenC] = None
> >           self._genh: Optional[QAPIGenH] = None
> > -        self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
> > +        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
> >           self._main_module: Optional[str] = None
> >   
> >       @staticmethod
> > -    def _is_user_module(name: Optional[str]) -> bool:
> > -        return bool(name and not name.startswith('./'))
> > +    def _is_user_module(name: str) -> bool:
> > +        return not name.startswith('./')
> >   
> >       @staticmethod
> > -    def _is_builtin_module(name: Optional[str]) -> bool:
> > -        return not name
> > +    def _is_builtin_module(name: str) -> bool:
> > +        return name == './builtin'
> >   
> > -    def _module_dirname(self, name: Optional[str]) -> str:
> > +    def _module_dirname(self, name: str) -> str:
> >           if self._is_user_module(name):
> >               return os.path.dirname(name)
> >           return ''
> >   
> > -    def _module_basename(self, what: str, name: Optional[str]) -> str:
> > +    def _module_basename(self, what: str, name: str) -> str:
> >           ret = '' if self._is_builtin_module(name) else self._prefix
> >           if self._is_user_module(name):
> >               basename = os.path.basename(name)
> > @@ -269,15 +269,14 @@ def _module_basename(self, what: str, name: Optional[str]) 
> > -> str:
> >               if name != self._main_module:
> >                   ret += '-' + os.path.splitext(basename)[0]
> >           else:
>
> Possible drive-by improvement:
>
>                 assert name.startswith('./')

As long as nobody tells me to split that unrelated change into a new 
patch I'll happily add drive-by improvements upon request ;)

> > -            name = name[2:] if name else 'builtin'
> > -            ret += re.sub(r'-', '-' + name + '-', what)
> > +            ret += re.sub(r'-', '-' + name[2:] + '-', what)
> >           return ret
> >   
> > -    def _module_filename(self, what: str, name: Optional[str]) -> str:
> > +    def _module_filename(self, what: str, name: str) -> str:
> >           return os.path.join(self._module_dirname(name),
> >                               self._module_basename(what, name))
> >   
> > -    def _add_module(self, name: Optional[str], blurb: str) -> None:
> > +    def _add_module(self, name: str, blurb: str) -> None:
> >           basename = self._module_filename(self._what, name)
> >           genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
> >           genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> > @@ -290,8 +289,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
> >               self._main_module = name
> >           self._add_module(name, blurb)
> >   
> > -    def _add_system_module(self, name: Optional[str], blurb: str) -> None:
> > -        self._add_module(name and './' + name, blurb)
> > +    def _add_system_module(self, name: str, blurb: str) -> None:
> > +        self._add_module(f"./{name}", blurb)
>
> I like f-strings, I really do, but is
>
>      f"./{name}"
>
> really clearer than
>
>      './' + name
>
> ?

I tend to avoid string concatenation as a habit, but there's no greater 
reason; the old value needed updating and that's what fell out of my 
keyboard, I suppose.

> >   
> >       def write(self, output_dir: str, opt_builtins: bool = False) -> None:
> >           for name in self._module:
> > @@ -310,7 +309,7 @@ def _begin_user_module(self, name: str) -> None:
> >       def visit_module(self, name: Optional[str]) -> None:
> >           if name is None:
> >               if self._builtin_blurb:
> > -                self._add_system_module(None, self._builtin_blurb)
> > +                self._add_system_module('builtin', self._builtin_blurb)
> >                   self._begin_system_module(name)
> >               else:
> >                   # The built-in module has not been created.  No code may