Re: [PATCH v3 06/17] qapi: centralize is_[user|system|builtin]_module methods

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> Define what a module is and define what kind of a module it is once and
> for all, in one place.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py |  2 +-
>  scripts/qapi/gen.py    | 21 +++++++--------------
>  scripts/qapi/schema.py | 28 ++++++++++++++++++++++++++--
>  3 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index f754f675d66..08b9439de2b 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -464,7 +464,7 @@ def __init__(self, env, qapidir):
>          self._qapidir = qapidir
>  
>      def visit_module(self, module):
> -        if module.name:
> +        if module.user_module:
>              qapifile = self._qapidir + '/' + module.name
>              self._env.note_dependency(os.path.abspath(qapifile))
>          super().visit_module(module)

Cleaner:

           qapifile = module.source_filename()
           if qapifile:
               self._env.note_dependency(os.path.abspath(qapifile))

Observation, not demand.

> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index e73d3d61aac..3b7eddc1c21 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -250,21 +250,14 @@ def __init__(self,
>          self._main_module: Optional[str] = None
>  
>      @staticmethod
> -    def _is_user_module(name: Optional[str]) -> bool:
> -        return bool(name and not name.startswith('./'))
> -
> -    @staticmethod
> -    def _is_builtin_module(name: Optional[str]) -> bool:
> -        return not name
> -
> -    def _module_dirname(self, name: Optional[str]) -> str:
> -        if self._is_user_module(name):
> +    def _module_dirname(name: Optional[str]) -> str:
> +        if QAPISchemaModule.is_user_module(name):
>              return os.path.dirname(name)
>          return ''

No use of self left.  @staticmethod?

Or maybe ...

>  
>      def _module_basename(self, what: str, name: Optional[str]) -> str:
> -        ret = '' if self._is_builtin_module(name) else self._prefix
> -        if self._is_user_module(name):
> +        ret = '' if QAPISchemaModule.is_builtin_module(name) else self._prefix
> +        if QAPISchemaModule.is_user_module(name):
>              basename = os.path.basename(name)
>              ret += what
>              if name != self._main_module:
               if name != self._main_module:
                   ret += '-' + os.path.splitext(basename)[0]
           else:
               name = name[2:] if name else 'builtin'
               ret += re.sub(r'-', '-' + name + '-', what)
           return ret

       def _module_filename(self, what: str, name: Optional[str]) -> str:
           return os.path.join(self._module_dirname(name),
                               self._module_basename(what, name))

... replace this by

           return os.path.join(module.source_dirname(),
                               self._module_basename(what, name))

Requires plumbing @module from visit_module() to here.

Again, not a demand.

> @@ -286,7 +279,7 @@ def _add_module(self, name: Optional[str], blurb: str) -> None:
>          self._genc, self._genh = self._module[name]
>  
>      def _add_user_module(self, name: str, blurb: str) -> None:
> -        assert self._is_user_module(name)
> +        assert QAPISchemaModule.is_user_module(name)
>          if self._main_module is None:
>              self._main_module = name
>          self._add_module(name, blurb)
> @@ -296,7 +289,7 @@ def _add_system_module(self, name: Optional[str], blurb: str) -> None:
>  
>      def write(self, output_dir: str, opt_builtins: bool = False) -> None:
>          for name in self._module:
> -            if self._is_builtin_module(name) and not opt_builtins:
> +            if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
>                  continue
>              (genc, genh) = self._module[name]
>              genc.write(output_dir)
> @@ -309,7 +302,7 @@ def _begin_user_module(self, name: str) -> None:
>          pass
>  
>      def visit_module(self, module: QAPISchemaModule) -> None:
> -        if module.name is None:
> +        if module.system_module:
>              if self._builtin_blurb:
>                  self._add_system_module(None, self._builtin_blurb)
>                  self._begin_system_module(module.name)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 69ba722c084..44a9232c881 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -141,6 +141,30 @@ def __init__(self, name):
>          self.name = name
>          self._entity_list = []
>  
> +    @classmethod
> +    def is_system_module(cls, name: Optional[str]) -> bool:
> +        return name is None or name.startswith('./')
> +
> +    @classmethod
> +    def is_user_module(cls, name: Optional[str]) -> bool:
> +        return not cls.is_system_module(name)
> +
> +    @classmethod
> +    def is_builtin_module(cls, name: str) -> bool:
> +        return name == './builtin'

The names suggest these are predicates on modules, but they're actually
predicates on module *names*.

@staticmethod would suffice.  Actually, *functions* would.  All the
"classiness" buys us here is having to write
QAPISchemaModule.is_FOO_module(name) instead of is_FOO_module(name), and
...

If you *want* classy, have a class QAPISchemaModuleName.

> +
> +    @property
> +    def system_module(self) -> bool:
> +        return self.is_system_module(self.name)
> +
> +    @property
> +    def user_module(self) -> bool:
> +        return self.is_user_module(self.name)
> +
> +    @property
> +    def builtin_module(self) -> bool:
> +        return self.is_builtin_module(self.name)

... module.FOO_module instead of module.is_FOO_module.

Any particular reason for the @property decorator?

> +
>      def add_entity(self, ent):
>          self._entity_list.append(ent)
>  
> @@ -871,8 +895,8 @@ def resolve_type(self, name, info, what):
>          return typ
>  
>      def _module_name(self, fname):
> -        if fname is None:
> -            return None
> +        if QAPISchemaModule.is_system_module(fname):
> +            return fname
>          return os.path.relpath(fname, self._schema_dir)
>  
>      def _make_module(self, fname):
           name = self._module_name(fname)
           if name not in self._module_dict:
               self._module_dict[name] = QAPISchemaModule(name)
           return self._module_dict[name]

I got one more issue.

Before the patch:

* QAPISchemaModule deals with the user's modules and a single, special
  module for built-ins.  The former's name is the schema module filename
  relative to the main schema's directory, the latter's name is None.

* QAPISchemaModularCVisitor supports both kinds, and adds a third kind:
  system modules.  Their name begins with './', which cannot clash.
  These guys are used for generated code that doesn't fit anywhere else.

Your patch drags system modules into QAPISchemaModule.  Somewhat
awkward.  Could use comments (but then, much of scripts/qapi/ could).

Subtyping QAPISchemaModule is yet another possible way to avoid putting
knowledge about QAPISchemaModularCVisitor's doings into
QAPISchemaModule.